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

fix: default value for target on action.yml #121

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Conversation

nuragic
Copy link
Contributor

@nuragic nuragic commented Dec 7, 2021

The default value for target was still major but it has been changed to any 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! 🙏

The default value for `target` was still `major` but it has been changed
to `any` here fastify#99.
@nuragic
Copy link
Contributor Author

nuragic commented Dec 7, 2021

Additional thought 💭

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 *

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

Additional thought 💭

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 *

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

@nuragic
Copy link
Contributor Author

nuragic commented Dec 7, 2021

I’d say that we should land this as a patch (2.7.1) maybe, then we can discuss the idea of renaming any to * for the upcoming major (v3) in a separate issue as you suggested @simoneb. What do you think @Eomm ?

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

If it fits into the v3 rollout plan defined by @Eomm it works for me

Copy link
Member

@Eomm Eomm left a 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 👍🏽

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

@nuragic create an issue for any -> * please

@simoneb simoneb merged commit c8f3391 into fastify:main Dec 7, 2021
@Eomm
Copy link
Member

Eomm commented Dec 7, 2021

This PR fixes this issue too:

Since the default was target: major in the action.yml configuration, this check was executed:

if (TARGET !== targetOptions.any) {

At the end this code executes:

var semver = require("semver/functions/diff")
const expression = /from ([^\s]+) to ([^\s]+)/
const title = 'chore(deps): bump nearform/optic-release-automation-action from 2.2.0 to 2.3'
const match = expression.exec(title)
const diff = semver(match[1], match[2])

Triggering the same error you can see on the GHA logs:

Invalid Version: 2.3

https://runkit.com/embed/7mev193gln7y

@Eomm Eomm mentioned this pull request Dec 7, 2021
2 tasks
@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

how does it solve the issue? won't we still have the issue if when using the action we use anything that's not any?

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

I think the real fix needs to be done and may have to be using coerce or loose options in semver

@Eomm
Copy link
Member

Eomm commented Dec 7, 2021

how does it solve the issue?

It solves because it turn it off the check

won't we still have the issue if when using the action we use anything that's not any?

This is correct: we can still make more solid this check accepting the target option.
I will open an issue for this.

@simoneb
Copy link
Collaborator

simoneb commented Dec 7, 2021

thanks @Eomm , that's the way to go

@Eomm Eomm mentioned this pull request Dec 7, 2021
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

Successfully merging this pull request may close these issues.

3 participants