Skip to content

feat: deprecate enforcesemver2 config option#522

Merged
jdolitsky merged 2 commits into
helm:mainfrom
cbuto:feat/deprecate-enforce-semver
Jan 25, 2022
Merged

feat: deprecate enforcesemver2 config option#522
jdolitsky merged 2 commits into
helm:mainfrom
cbuto:feat/deprecate-enforce-semver

Conversation

@cbuto

@cbuto cbuto commented Jan 22, 2022

Copy link
Copy Markdown
Contributor

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?

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

@cbuto cbuto Jan 22, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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

@jdolitsky jdolitsky left a comment

Copy link
Copy Markdown
Contributor

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 thread pkg/chartmuseum/server.go Outdated
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
Copy Markdown
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
Copy Markdown
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 👍

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

Copy link
Copy Markdown
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

@jdolitsky jdolitsky left a comment

Copy link
Copy Markdown
Contributor

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

scbizu commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

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