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

Validate the module version as version instead of ID #409

Merged
merged 1 commit into from Oct 31, 2022

Conversation

svanharmelen
Copy link
Collaborator

Without these changes a module version with prerelease or metadata will fail, while these are actually valid SemVer versions that are allowed by the module registry.

The PR includes tests that fail without the additional code changes, so everything that is needed should be in here...

sebasslash
sebasslash previously approved these changes May 25, 2022
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thank you so much for these changes 🔥

_, err := mail.ParseAddress(v)
// validVersion checks if the given input is a valid version.
func validVersion(v string) bool {
_, err := version.NewVersion(v)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a neat trick! Wasn't aware we could leverage this, very cool :D

@svanharmelen
Copy link
Collaborator Author

Rebased to fix the merge conflicts...

sebasslash
sebasslash previously approved these changes Jun 8, 2022
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! ⚡

@annawinkler
Copy link
Contributor

Hi @svanharmelen - could you please update your fork with main? We'd like to get this PR merged. Thank you! 🥳

@svanharmelen
Copy link
Collaborator Author

@annawinkler done... Noticed the integration tests failed, but its because of a missing token so don't think it's related to the changed in the PR 🤷🏻‍♂️

@annawinkler
Copy link
Contributor

@svanharmelen Thank you! Yes, we'll take a look at that.

annawinkler
annawinkler previously approved these changes Aug 17, 2022
Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@fatbasstard
Copy link

@annawinkler It is approved now, can this also be merged and released? Would be nice if we can remove our forked repo out of ecosystem 😉

Without these changes a module version with prerelease or metadata will fail, while these are actually valid SemVer versions that are allowed by the module registry.
@annawinkler
Copy link
Contributor

Opened a new PR #571 so I can get tests to run. Will close this PR after the new one is merged.

@annawinkler annawinkler merged commit 72de0b5 into hashicorp:main Oct 31, 2022
@github-actions
Copy link

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

annawinkler added a commit that referenced this pull request Oct 31, 2022
annawinkler added a commit that referenced this pull request Oct 31, 2022
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.

None yet

4 participants