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

refactor: remove easyjson dependency #34

Merged
merged 5 commits into from
Jul 31, 2023

Conversation

fabriziosestito
Copy link
Contributor

@fabriziosestito fabriziosestito commented Jul 28, 2023

Description

Removes the easyjson dependency by bumping policy-sdk to the latest version.

Test

Existing tests should pass.

Additional Information

Artifact hub configuration was updated.

Related to: kubewarden/kubewarden-controller#492

settings.go Outdated
Comment on lines 14 to 18
type RawSettings struct {
DeniedLabels []string `json:"denied_labels"`
MandatoryLabels []string `json:"mandatory_labels"`
ConstrainedLabels map[string]string `json:"constrained_labels"`
}
Copy link
Member

Choose a reason for hiding this comment

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

We should merge RawSettings with Settings, this was just a workaround caused by easyjson not supporting properly mapset. Can you give a shot at unmarshalling the json straight into Settings?

Copy link
Contributor Author

@fabriziosestito fabriziosestito Jul 28, 2023

Choose a reason for hiding this comment

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

refactored in: 2fd4d59
Implemented json.Unmarshaler for Settings, but kept an anonymous struct similar to RawSettingsto simplify the unmarshalling.

Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM, modulo Flavio's comment.

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

I love the new approach to handle Settings and RawSettings 👏

@flavio
Copy link
Member

flavio commented Jul 31, 2023

I'm going to merge it, so that I can trigger a rebase of #35 and merge it. Please wait for #35 to be merged before tagging a new release

@flavio flavio merged commit 858eca6 into kubewarden:main Jul 31, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants