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

Target minor as default #119

Closed
2 tasks done
Eomm opened this issue Dec 7, 2021 · 9 comments
Closed
2 tasks done

Target minor as default #119

Eomm opened this issue Dec 7, 2021 · 9 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers

Comments

@Eomm
Copy link
Member

Eomm commented Dec 7, 2021

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Starting from this issue:

The same issue above happened to the fastify's modules as well.

I think the target: minor is a more secure default for users.

Or, we could define target: major per dev-dependancies and target: minor for dependencies.

Motivation

Usually, the merge PRs are overseen and you may release a wrong semver or face some issue that needs investigating a lot.

There was already a comment somewhere that was cons to this change, but I think worth discussing it a bit.

Example

No response

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

I do not agree with this proposal because when doing things in the right way, meaning the code is tested, bumping majors is perfectly fine because breakages will be caught by tests. If we were to do this change, we would favor users who are not doing things in the right way and put more burden on users who are (because either they would need to manually merge those PRs or they have to do additional configuration when using the action).

Hence, I think we should prioritize users who are doing things in the right way by keeping "any" as the default.

Also note that minor is not a sensible default in general because for submodule bumps, we don't have any versions (see #95), hence why we recently set it to "any".

@Eomm
Copy link
Member Author

Eomm commented Dec 7, 2021

What I see are two usages:

  1. per application: a major is fine as you highlight
  2. per module: a major is not (always) fine because the module may simple be exported to the user, changing the API actually
  3. per major/minor tags the target must be any (fix: default value for target on action.yml #121 (comment))

Do you agree on these two types?
I think documenting it would be fine

EDIT: add 3rd option

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

I think it makes more sense to put this on hold until there are less things moving around here, post v3

@simoneb simoneb added the enhancement New feature or request label Dec 10, 2021
@Eomm Eomm added documentation Improvements or additions to documentation good first issue Good for newcomers labels Dec 13, 2021
@kmthorsnes
Copy link

Sorry for hijacking, but this seemed like a relevant place to ask:

I want to limit the automerging to be minor and patches:

I tried:
target: minor || patch
But a major update was merged with this setting.

Would using:
target: minor also adress that patches would be merged, but not major?

@simoneb
Copy link
Collaborator

simoneb commented Oct 5, 2023

@kmthorsnes correct, and indeed the docs don't clarify it, but when you set it to minor, anything "less" than minor will be merged. We should probably clarify it in the docs

@kmthorsnes
Copy link

That clears things up, great!

Thank you for swift reply 🚤 !

@jmullo
Copy link

jmullo commented Oct 11, 2023

@simoneb
Copy link
Collaborator

simoneb commented Oct 11, 2023

@jmullo this feels like a different feature request, please open a new one

@simoneb
Copy link
Collaborator

simoneb commented Oct 11, 2023

I'm going to close this one which has been open for two years. Although somebody will disagree, it's clear that we're not going to do this, therefore no point in keeping it open.

If you want to target minor, use the corresponding action input.

@simoneb simoneb closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants