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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the default value for target from any to * #122

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

Change the default value for target from any to * #122

nuragic opened this issue Dec 7, 2021 · 9 comments

Comments

@nuragic
Copy link
Contributor

nuragic 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

Moving the discussion from #121 here:

As stated in the README:

based on Semantic Versioning

any is not valid in SemVer, but we have * for that, so I guess we should replace any for *.

@nuragic nuragic changed the title Change the default value for target fromany to * Change the default value for target from any to * Dec 7, 2021
@Eomm
Copy link
Member

Eomm commented Dec 7, 2021

At the moment target: any means:

the action does not check the semver before approve&merge

Instead, target: * could mean:

before approve&merge check if the semver has a right format

This is a different meaning for me.

A quick search about these GHA users let me think if all these repos rely on semver.

I would rename any to disabled instead

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

again the main thing to consider here is supporting submodules which don't have a semver version as per #95

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

see also conversation in #121 for context

@nuragic
Copy link
Contributor Author

nuragic commented Dec 9, 2021

Right now target: any means any target, so put in other words: the check is actually disabled, right?

The * rename proposal was to align to the SemVer spec (as we state in the docs, target needs to be a valid SemVer identifier), because it literally means any version.

However, given the submodule requirement (target is a git hash, so not a valid SemVer neither), the right thing to do IMHO would be to just leave target undefined by default, because it's an optional field and it doesn't really need to have a value, I think it would be easiest to understand in this way (no target by default, no check).

@simoneb
Copy link
Collaborator

simoneb commented Dec 9, 2021

Well the change is already merged and released but this issue is still open because it wasn't linked from the PR

@simoneb
Copy link
Collaborator

simoneb commented Dec 10, 2021

@nuragic can you please acknowledge the message above? If so, can you please close this issue?

@nuragic
Copy link
Contributor Author

nuragic commented Dec 10, 2021

I agree about that it doesn't make sense to use * so I'll close it now.

But I'd like to address this proposal.

Given the submodule requirement (target is a git hash, so not a valid SemVer neither), the right thing to do IMHO would be to just leave target undefined by default, because it's an optional field and it doesn't really need to have a value, I think it would be easiest to understand in this way (no target by default, no check).

Maybe in a new issue?

@nuragic nuragic closed this as completed Dec 10, 2021
@simoneb
Copy link
Collaborator

simoneb commented Dec 10, 2021

Yes, thanks!

@nuragic
Copy link
Contributor Author

nuragic commented Dec 13, 2021

Moved to #128

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

No branches or pull requests

3 participants