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.

Sign up for GitHub

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

Jump to bottom

Issue66 - Add CI parameter #67

Closed
wants to merge 2 commits into from
Closed

Issue66 - Add CI parameter #67

wants to merge 2 commits into from

Conversation

Biboba
Copy link

@Biboba Biboba commented Feb 25, 2019 •
edited by leonardoanalista

If user specify --ci parameter, then no "[ci skip] NO_CI" is not included in commit message

It closes #66

Nicolas Guilhaudin added 2 commits February 25, 2019 18:38
feat(app): add ci parameter
da75cc6 
users can specify whether or not CI scripts should be run
test(app): test for --ci
7a64509 
unit test for --ci parameter
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 93.725% when pulling 7a64509 on Biboba:issue66 into 522b239 on leonardoanalista:master.

leonardoanalista
leonardoanalista reviewed Feb 25, 2019
View reviewed changes
src/lib/addFilesAndCreateTag.js
let code;
// ###### Add edited files to git #####
log.info('>>> About to add and commit package.json and CHANGELOG...');
code = shell.exec('git add package.json CHANGELOG.md').code;
terminateProcess(code);

// ###### Commit files #####
code = shell.exec('git commit -m "chore(release): ' + newVersion + ' [ci skip] ***NO_CI***"').code;
let commitMessage = 'git commit -m "chore(release): ' + newVersion;
if (!ci) {
Copy link
Owner

Choose a reason for hiding this comment

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

Hi @Biboba Thanks for the PR first of all. I have a few questions here:

  • semantic-release has option ci default set to true - it means it only adds [ci skip] if we set it to --ci. In your case you have the opposite condition here.
  • I also find very confusing the short name ci. I know we agreed to this variable name but what do you think if we use ci-skip or no-ci? Second mirrors semantic-release api.
  • could we make it default true, like semantic-release. If so, we need to make it a Breaking Change in this version and analyze the impact of the new version.
  • Would you mind updating the README with the new version? I know it's a duplication of the program.options. I am open for suggestions here too to have only one place where we modify the options and keep readme updated.

refs:
https://github.com/semantic-release/semantic-release/blob/caribou/docs/usage/configuration.md#ci

@leonardoanalista
Copy link
Owner

Hi @uglow how are you? If you still use this tool, feel free to give your opinion too.

@uglow
Copy link
Collaborator

uglow commented Feb 26, 2019

@leonardoanalista - I’m good thanks, maybe hit me up on LinkedIn for a chat.

@Biboba, nice PR 👍🏻
I think it could be improved if it follows the conventions @leonardoanalista referred to above.

@Biboba
Copy link
Author

Biboba commented Feb 26, 2019

Hello,

Happy to make the changes.

But I had a closer look to semantic-release package and this --ci option is not exactly what I thought it is : it does not involve any "[ci skip] NO_CI" message.
It is rather an option to be able to make a local release without using any CI (Travis, Gitlab,...). It is basically what this plugin does.

Apologize for the confusion but still I think it would be interesting to be able to specify "NO_CI" as an option. I don't know if it changes the way you feel about this PR.

Thanks for the feedback !

References:

@leonardoanalista
Copy link
Owner

leonardoanalista commented Mar 1, 2019 •
edited

Thanks @Biboba for your research. Considering we are talking about different options between semantic-release and this package, we could do whatever we want here. I think now we have a better understanding of this new option.

  • We can avoid the breaking change. In this case, we could call this option ci-skip instead of ci
  • Make ci-skip default false to avoid the breaking change.
  • Option ci-skip will allow local release as every CI skips the build if commit message contains [ci skip]
  • Update read me to have the new option: https://github.com/leonardoanalista/corp-semantic-release#options

What do you think about the points above?

@FranzNieschalk FranzNieschalk mentioned this pull request Mar 19, 2019
Merged
leonardoanalista pushed a commit that referenced this pull request Mar 19, 2019
@leonardoanalista
* feat(app): add skip-ci parameter (#71)
8293280 
Users can specify whether or not CI scripts should be run. Default is true - skip CI

BREAKING CHANGE:
Commit message for files `package.json` and `CHANGELOG` will contain text `[ci skip] ***NO_CI***'` so CI's can skip a build for this commit.

ISSUES CLOSED: issue #66 
PRs CLOSED: closes PR #67
@leonardoanalista
Copy link
Owner

addressed by #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonardoanalista leonardoanalista leonardoanalista left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI option
4 participants
@Biboba @coveralls @leonardoanalista @uglow