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

feat: deprecate enforcesemver2 config option #522

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

cbuto
Copy link
Contributor

@cbuto cbuto commented Jan 22, 2022

This is a follow up on #482 to fully "deprecate" the --enforce-semver2 flag. It seems like some of the documentation work is capture in #485, which I can follow up on.

Also, do we want a way to mark config options as deprecated and print warnings if they are set on startup?

if err != nil {
// TODO: this is an error if invalid semver, making "EnforceSemver2" useless...
return filename, &HTTPError{http.StatusInternalServerError, err.Error()}
Copy link
Contributor Author

@cbuto cbuto Jan 22, 2022

Choose a reason for hiding this comment

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

💭 While I get familiar with everything, should we return a 400 here and skip the call to ChartVersionFromStorageObject entirely? We don't need the version for anything anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, fixed in 2f11e50!

@cbuto cbuto force-pushed the feat/deprecate-enforce-semver branch from cdd5fec to d4fc0e8 Compare January 22, 2022 20:58
Copy link
Contributor

@jdolitsky jdolitsky left a comment

Choose a reason for hiding this comment

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

do we want a way to mark config options as deprecated and print warnings
yes, that would be great. Thanks @cbuto!

Comment on lines 139 to 142
// EnforceSemver was deprecated, see https://github.com/helm/chartmuseum/issues/485 for more info
EnforceSemver2: options.EnforceSemver2,
Version: options.Version,
CacheInterval: options.CacheInterval,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you place it at the bottom of the struct, so we can have a section for deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 2f11e50 👍

if err != nil {
// TODO: this is an error if invalid semver, making "EnforceSemver2" useless...
return filename, &HTTPError{http.StatusInternalServerError, err.Error()}
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that sounds good

@cbuto cbuto force-pushed the feat/deprecate-enforce-semver branch from 4894cdc to 2f11e50 Compare January 24, 2022 14:25
Copy link
Contributor

@jdolitsky jdolitsky left a comment

Choose a reason for hiding this comment

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

👍

@jdolitsky jdolitsky added this to the v0.14.0 milestone Jan 25, 2022
@jdolitsky jdolitsky merged commit c08bf65 into helm:main Jan 25, 2022
@scbizu
Copy link
Contributor

scbizu commented Jan 25, 2022

Cool @cbuto , it really do me a favor 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants