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

Ensuring every build is deterministic by adding npm ci as a standard #10789

Merged
merged 18 commits into from
Aug 13, 2019

Conversation

Amoenus
Copy link
Contributor

@Amoenus Amoenus commented Jun 28, 2019

More information about ci command can be found here


This change is Reviewable

@Amoenus Amoenus requested review from jotaylo and zjrunner as code owners June 28, 2019 10:00
@Amoenus
Copy link
Contributor Author

Amoenus commented Jul 19, 2019

@zjrunner @jotaylo would welcome your feedback on this PR

@zjrunner
Copy link
Member

logically seems fine. We're working on pushing people toward an authentication task and then letting them have the freedom of directly using npm and whatever commandlines they need so generally I wouldn't want to be adding to these tasks. I'd have bigger concerns if the default option were changing, as-is I just need to think about additional support/confusion a bit given the path we're on.

@zjrunner
Copy link
Member

@Amoenus - ah, I missed you were changing the default - I initially had a big pushback to that but then thought you weren't changing it. Definitely move the default back to what it was originally as we don't want any additional confusion out of this task given the context I gave earlier.

@Amoenus
Copy link
Contributor Author

Amoenus commented Jul 22, 2019

@zjrunner brought the default back to 'install'. So should be good to go.

Could you elaborate on the push towards npm authenticate task?

It seems counter-intuitive to go from a well-defined step like Npm to a vaguer command line for example.

I see from the comment descriptions in the npm authenticate task it is assumed that npm is used within other tasks like Gulp, but that is not always the case, so I see a lot of value in Npm task.

@zjrunner
Copy link
Member

@Amoenus , the 'heavy' tasks provide auth value and often a little other wrapping value, but also get in the way quite often from folks using other parts of native tooling. Beyond blocked features, there are also introduced bugs, version-over-version compat issues (of the task or the underlying tooling), and other potential issues. By separating out auth we want folks to be able to have the direct access to the full commandlines for npm or other tooling (yarn, etc). We also want to enable and better support the YAML flows in Pipelines, in which folks are moving to a more direct commandline flow with native tooling in a more streamlined way -- the auth task plays into this nicely in supporting many ways of working.

@Amoenus
Copy link
Contributor Author

Amoenus commented Jul 30, 2019

@zjrunner many thanks for the explanation. I see how version differences can mess up the subsequent tasks. Encountered it myself and needed to lock down the specific node version for a custom script to work. I will always advocate for removing the obstacles for people, so still will strive for wrapping long commands into a nice "DoComplexWorkTheEasyWay" button.

I guess like with all the things the truth is something in the middle and once the common workflows are established using the proposed auth+cmd route, potential new task templated will become apparent :)

Oh and separate thank you to you for approving my PR. Can't wait to get it merged in ^_^

@Amoenus
Copy link
Contributor Author

Amoenus commented Aug 13, 2019

@zjrunner should I bump the patch version for the task before you merge it or will it be done at a later stage in the main repo?

@zjrunner zjrunner merged commit 5d72a51 into microsoft:master Aug 13, 2019
@zjrunner
Copy link
Member

normally rev the version with the change, but sometimes we have a pending release and don't need to rev. I committed since this has been outstanding for a while and I'll talk with someone on my team to rev the version if needed so it gets in the release pipeline.

@Amoenus Amoenus deleted the feature/npmci branch August 15, 2019 13:22
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.

2 participants