CircleCI-Public / orb-tools-orb

Various tools for authoring and publishing CircleCI orbs
https://circleci.com/orbs/registry/orb/circleci/orb-tools
MIT License
51 stars 74 forks source link

semver default value for orb-tools/dev-promote-prod-from-commit-subject #77

Closed nik-shornikov closed 3 years ago

nik-shornikov commented 4 years ago

the ideal place to control publishing based on commits to master is exactly what this job expects, but a finer control than/in addition to fail-if-semver-not-indicated would be e.g. semver-default (whose own default might be skip) this would allow for greater flexibility with workflows, and probably a good number of users would opt to change the default to patch (were probably not the only ones calling this job with a branch filter) im torn between submitting that PR and doing some pre-steps that just git commit --amend -m "[semver:$DEFAULT]" for this job in our workflow

nik-shornikov commented 4 years ago

https://github.com/CircleCI-Public/orb-tools-orb/blob/e302710073ca365243bf990bfe3dfb66573843a1/src/commands/dev-promote-from-commit-subject.yml#L92-L100

lokst commented 4 years ago

@nik-shornikov Thanks for sharing your feedback! The approach taken in designing the command was so that orb publishers would be intentional about the semver increment appropriate for the change. Do let us know if there are any concerns or difficulties arising from this 🙂

nik-shornikov commented 4 years ago

well, the difficulty is just the human element -- it would be easier if there was some way to set a default (which itself would default to "skip" ie. you must be intentional)

in the unlikely scenario of a one-branch repo, changing the default behavior to do something other than a skip would not be a great idea, but in our case, this job only runs on master and all the work is on feature branches (for us something getting committed to master without a publish intended "isnt a thing")

so if someone forgets to add the magic string (or gets confused about at which moment to do that), we end up having to do empty commits or some other silliness

bmbferreira commented 3 years ago

Hi! I also think that this is an important parameter to have. My colleagues are always forgetting to put the increment type on the commit subject and we're tired of having to do new commits just to bump the version. By default we should just fallback to patch for example. Please consider the PR I just opened and please tell me if you want me to do it in a different way! Thanks!

KyleTryon commented 3 years ago

Hello all,

@nik-shornikov and @bmbferreira I'd like to hear more about your use case here and ensure I fully understand. As @lokst mentioned, the existing design is set as so to force the developer to make a conscious choice as to the type of release to create. We have opted to use the commit message, the other sensible option we have considered (and I am still open to) is to use git tags. Typically with GitHub releases, you would need to intentionally (and normally manually) specify the tag and release type. This is important in preventing a scenario for instance wherein the developer commits a breaking change to the main branch and having that change automatically publish as a minor or patch release which would result in failed pipelines for users of the orb.

I am cautious to add any default parameters which would seem to allow the users to potentially violate semver versioning, which could be damaging to the end-user. I believe that specifying a default value would be a poor release practice.

this job only runs on master and all the work is on feature branches (for us something getting committed to master without a publish intended "isnt a thing")

Ah, I am thinking perhaps you are using the orb in a different pattern than I expected which might explain this. Would you be willing to share any of your config?

My assumptions are going based on this example: https://github.com/CircleCI-Public/Orb-Project-Template/blob/8113e8e4c3f54898d90360208222d3d5a5aad1f3/.circleci/config.yml#L89

      - orb-tools/dev-promote-prod-from-commit-subject:
          orb-name: <namespace>/<orb-name>
          context: <publishing-context>
          add-pr-comment: false
          fail-if-semver-not-indicated: true
          publish-version-tag: false
          requires:
            - integration-test-1
          filters:
            branches:
              only:
                - master
                - main

This is the default config template when creating a new orb with the orb devkit. Features are created and tested in separate branches, but they are published when they are merged into the main branch. This workflow will build on all commits to the main branch but we are assuming you are only merging in from feature branches.

You can see in this setup, because we publish when merging, we must know the type of publishing.

bmbferreira commented 3 years ago

Hi @KyleTryon

@nik-shornikov and @bmbferreira I'd like to hear more about your use case here and ensure I fully understand. As @lokst mentioned, the existing design is set as so to force the developer to make a conscious choice as to the type of release to create. We have opted to use the commit message, the other sensible option we have considered (and I am still open to) is to use git tags. Typically with GitHub releases, you would need to intentionally (and normally manually) specify the tag and release type.

Ideally this wouldn't be done manually and we would use a tool (semantic release or git version, for example) to automatically calculate the version from the commit history. If we do not force a convention, the version bumping won't be consistent and what is a minor for some, will be a patch for others, and so on. So basically, by "forcing" it in the commit message we are not really forcing anything. People will forget to put it on the message because it is not automated and then they realize that the version was not bumped and will have to do a new empty PR just to bump it. Which is a pain 😢 If I had a way to set a template commit subject for PR merges, I wouldn't be opening this PR to this orb. I would just add [semver:patch] to the template and that's it.

This is important in preventing a scenario for instance wherein the developer commits a breaking change to the main branch and having that change automatically publish as a minor or patch release which would result in failed pipelines for users of the orb. I am cautious to add any default parameters which would seem to allow the users to potentially violate semver versioning, which could be damaging to the end-user. I believe that specifying a default value would be a poor release practice.

The developer might not even realize that he's doing a breaking change and just put [semver:minor] in the subject. So, IMO a default value will not increase this issue. 😕

this job only runs on master and all the work is on feature branches (for us something getting committed to master without a publish intended "isnt a thing")

Ah, I am thinking perhaps you are using the orb in a different pattern than I expected which might explain this. Would you be willing to share any of your config?

I don't think he's using a different pattern 🤔 What he means is that when someone merges to master, he is sure that a new version should be published. So instead of assuming "skip" by default, I think he also wants to assume a default such as patch, major or minor.

gmemstr commented 3 years ago

Adding on some thoughts now that I've had more time to think on this.

My initial comment on the PR was related to user demand; however, upon further reflection, I've go a few more feelings in the opposite direction.

  1. This is our opinionated approach to writing and publishing an orb (this is perhaps the biggest point I want to make clear)
  2. There doesn't appear to be widespread demand for this outside of this issue (this is a bit of a flaky reason - there may be more demand that we simply aren't observing)
  3. Semver in the way we're enforcing is, for all intents and purposes, good practice given orbs are immutable

I do realise the frustration that forgetting to set the semver tag for the commit can be -- we've all been there :) But I think that it's better practice than relying on a default value that developers will unconsciously adopt into their workflow, rather than consciously setting a version for their update. Plus, there's nothing preventing this from being implemented in a custom configuration/fork of this orb.