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

Better npm ci integration #599

Merged
merged 2 commits into from
Nov 25, 2019
Merged

Better npm ci integration #599

merged 2 commits into from
Nov 25, 2019

Conversation

rlindner81
Copy link
Contributor

Description

For my current project I could use none of these pre-defined builders. So here is they way I would set it up for consideration.

Checklist

  • Code compiles correctly
  • Relevant tests were added (unit / contract / integration)
  • Relevant logs were added
  • Formatting and linting run locally successfully
  • All tests pass
  • UA review
  • Design is documented
  • Extended the README / documentation, if necessary
  • Open source is approved

For my current project I could use none of these pre-defined builders. So here is they way I would set it up for consideration.
commands:
- command: npm clean-install --production

- name: npm-ci-dev
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your contribution!

npm-ci looks good to me and commonly used, however, grunt-ci and npm-ci-dev should be handled as a custom builder as they are less common for production use and we want to avoid pollution of builders.
in addition, as this config is transferred to binary, please run the following command on the root to update the builder binary fiile

//go:generate go run ./internal/buildtools/embed.go ./internal/buildtools/buildtools_msg.go -source=./configs/builder_type_cfg.yaml -target=./internal/commands/builder_type_cfg.go -name=BuilderTypeConfig -package=commands
if you have an issue please let me know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included npm-ci-dev only because you already had it. I agree that this is one is mostly nonsense in production. grunt-ci on the other hand is the only sane way to run grunt in my opinion. The problem with not using clean install is that you introduce changes completely outside of your control into your pipeline.
Either way, I can run custom builders so that's not a request just my opinion.

I will run the update on Monday

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2019

CLA assistant check
All committers have signed the CLA.

@rlindner81
Copy link
Contributor Author

@ShimiT please re-review

Copy link
Collaborator

@ShimiT ShimiT left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@ShimiT ShimiT merged commit 9fbd4cd into SAP:master Nov 25, 2019
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.

None yet

3 participants