Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use docker to validate packages in js docindex #18405

Merged
merged 12 commits into from
Nov 5, 2021
Merged

Conversation

sima-zhu
Copy link
Contributor

@sima-zhu sima-zhu commented Oct 27, 2021

We plan to wrap the CI build command into docker image and use it to validate packages in docker.

The PR is to replace the sanity check first. Will have a follow up PR for creating issues whenever the package got filtered out.

Here is the testing pipeline:
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1174856&view=logs&j=dc056dfc-c0cf-5958-c8c4-8da4f91a3739

Test on no image id:
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1180723&view=logs&j=dc056dfc-c0cf-5958-c8c4-8da4f91a3739&t=bfb6cede-7671-5504-0a11-79b13ef63703

@sima-zhu sima-zhu requested a review from weshaggard October 29, 2021 16:48
Copy link
Member

@danieljurek danieljurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One relatively small change and I think we're good here. We do need to support folder so we don't unnecessarily remove packages from onboarding.

A trip down memory lane: This is the first pipeline for which I built package validation. It performs validation against ALL packages. This means we can expect the runtime of this pipeline to be more than 30 minutes given that we have to pass through the validation process twice (once for regular onboarding updates in main and again in the daily docs branch)... For now we can push the start time for the pipeline back but run time will continue to grow linearly as we add more packages... Fortunately the other 3 cardinal languages do not have this problem.

The long term fix is already opened here: #16626

Copy link
Member

@danieljurek danieljurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to leave it at Request changes instead of Approve given that we need to support folder

ghost pushed a commit to Azure/azure-sdk-tools that referenced this pull request Nov 2, 2021
Azure/azure-sdk-for-js#18405

Docs is switching the validation using docker container. Here is the common logic to pull image from Azure container register.

Testing pipeline in use with the script:
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1172146&view=logs&j=dc056dfc-c0cf-5958-c8c4-8da4f91a3739&t=bfb6cede-7671-5504-0a11-79b13ef63703
@sima-zhu
Copy link
Contributor Author

sima-zhu commented Nov 2, 2021

/check-enforcer override

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready once you finish your testing.

@sima-zhu
Copy link
Contributor Author

sima-zhu commented Nov 4, 2021

@danieljurek do you have time to review the PR? I will merge it after a test run and keep an eye on the performance once gets check in if no further response.

@sima-zhu
Copy link
Contributor Author

sima-zhu commented Nov 5, 2021

Done with testing. Testing link pasted in description.

@sima-zhu sima-zhu enabled auto-merge (squash) November 5, 2021 05:13
@sima-zhu sima-zhu dismissed danieljurek’s stale review November 5, 2021 05:13

Address all comments

@sima-zhu sima-zhu merged commit 6f2d579 into Azure:main Nov 5, 2021
@sima-zhu sima-zhu deleted the js_docker branch November 5, 2021 05:14
Copy link
Member

@danieljurek danieljurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

sadasant pushed a commit to sadasant/azure-sdk-for-js that referenced this pull request Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants