-
Notifications
You must be signed in to change notification settings - Fork 35
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
fix: default value for target on action.yml #121
Conversation
The default value for `target` was still `major` but it has been changed to `any` here fastify#99.
Additional thought 💭 As stated in the README:
|
Yes I like this idea! @Eomm shall we include it in the v3 which is coming up? @nuragic let's create an issue for this. Also @Eomm how do we get this fix in the current v2? We don't strictly have to if we prefer to postpone it until v3, but if we do let's do the "proper" fix by using * instead of any. Let's also consider that when dealing with submodules we're not dealing with semver, so we need to decide if * is really better than "any" in that scenario |
If it fits into the v3 rollout plan defined by @Eomm it works for me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good to release it as 2.7.1.
Let's discuss the any
value in an issue 👍🏽
@nuragic create an issue for any -> * please |
This PR fixes this issue too: Since the default was
At the end this code executes:
Triggering the same error you can see on the GHA logs:
|
how does it solve the issue? won't we still have the issue if when using the action we use anything that's not |
I think the real fix needs to be done and may have to be using coerce or loose options in semver |
It solves because it turn it off the check
This is correct: we can still make more solid this check accepting the |
thanks @Eomm , that's the way to go |
The default value for
target
was stillmajor
but it has been changed toany
here #99.Docs has been fixed as well, however I'm not sure about adding new tests... it would be great to have tests in place to prevent bugs like this in the future i.e. ensure that the documented defaults are correct.
Thanks!
This is my first PR here so sorry in advance if anything is missing! 🙏