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.

Sign up for GitHub

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

Jump to bottom

refactor: remove magic strings in struct tags #2787

Closed
wants to merge 22 commits into from
Closed

refactor: remove magic strings in struct tags #2787

wants to merge 22 commits into from

Conversation

AustinAbro321
Copy link
Contributor

@AustinAbro321 AustinAbro321 commented Jul 30, 2024 •
edited
Loading

Description

Fixes #2786

This should be merged after #2781

Checklist before merging

AustinAbro321 and others added 18 commits July 29, 2024 16:49
@AustinAbro321
switching json description to go comments
2d57404 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
switching json description to go comments
e466577 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
switching json description to go comments
a54b4d6 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
switching json description to go comments
d746067 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
make gen types work again
7e16860 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
wording
8dfe1a5 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
add json tags back
be388f8 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
add back json tags
59bc21f 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
inline
cde411a 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
Merge branch 'main' into utilize-invopop-features
8f2d3ec
@AustinAbro321
back to omitempty
fd680fc 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
add back test
cbc25f2 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
removing json options
da9ea48 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
add back json data
e8a11ac 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
bb required
de0bd7c 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
de-dup
96d703d 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
add period to json descriptions
380d2e7 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
use more invopop features
5e91b84 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321 AustinAbro321 requested review from a team as code owners July 30, 2024 20:50
@AustinAbro321 AustinAbro321 changed the title Use more invopop features refactor: use more invopop features Jul 30, 2024
@AustinAbro321 AustinAbro321 changed the title refactor: use more invopop features refactor: use invopop features to limit magic strings Jul 31, 2024
@AustinAbro321 AustinAbro321 changed the title refactor: use invopop features to limit magic strings refactor: remove magic strings in struct tags Jul 31, 2024
@AustinAbro321
merge
4623784 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@codecov Codecov
Copy link

codecov bot commented Jul 31, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 24 lines in your changes missing coverage. Please review.

Files Patch % Lines
src/types/component.go 0.00% 12 Missing ⚠️
src/pkg/variables/types.go 0.00% 8 Missing ⚠️
src/types/package.go 0.00% 4 Missing ⚠️
Files Coverage Δ
src/pkg/cluster/injector.go 71.89% <ø> (+0.41%) ⬆️
src/types/validate.go 92.95% <ø> (ø)
src/types/package.go 0.00% <0.00%> (ø)
src/pkg/variables/types.go 0.00% <0.00%> (ø)
src/types/component.go 12.50% <0.00%> (-7.50%) ⬇️

... and 1 file with indirect coverage changes

AustinAbro321 and others added 2 commits July 31, 2024 11:33
@AustinAbro321
Merge branch 'main' into use-more-invopop-features
5c35b98
@AustinAbro321
fix comments
d6ff4b0 
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321 AustinAbro321 mentioned this pull request Jul 31, 2024
Merged
2 tasks
phillebaba
phillebaba reviewed Jul 31, 2024
View reviewed changes
src/pkg/variables/types.go

// JSONSchemaExtend extends the generated json schema during `zarf internal gen-config-schema`
func (Variable) JSONSchemaExtend(schema *jsonschema.Schema) {
kind, _ := schema.Properties.Get("type")
Copy link
Member

Choose a reason for hiding this comment

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

Is this ignoring an error 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ignoring the present flag but I'd rather have the code panic if it doesn't find something since this is only run on zarf internal gen-config-schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't change the signature to return an error, since this signature is necessary for this function to get called by the jsonschema package during schema creation

Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot be a production grade tool that panics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is only called during schema generation, zarf internal gen-config-schema, and this would only panic if, during development, someone changed the json tag to not equal what is in the schema.Properties.Get call.

Given that we don't have control over this function signature I think there's two options:

  1. return when the key isn't found - IMO this is worse because there won't be any signal from the CLI that something went wrong during schema generation. I'm fine with this though, it would happen rarely & it should be relatively obvious from the git diff that something went wrong.
  2. Keep panicing, maybe explicitly panic with a clear error message.

@AustinAbro321
Copy link
Contributor Author

Closing this as we decided we don't like the fact that it's possible for jsonschemextend to panic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schristoff schristoff schristoff left review comments

@phillebaba phillebaba phillebaba left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor the types to clean up json / jsonschema tags
3 participants
@AustinAbro321 @phillebaba @schristoff