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

Include required properties in --default-values-file-output #1145

Merged
merged 3 commits into from
May 30, 2023
Merged

Include required properties in --default-values-file-output #1145

merged 3 commits into from
May 30, 2023

Conversation

praveenrewar
Copy link
Member

@praveenrewar praveenrewar commented Mar 21, 2023 •
edited
Loading

What this PR does / why we need it:

Include required properties in --default-values-file-output

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?

`--default-values-file-output` will also include required properties.

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:

@praveenrewar
Set a default namespace in test-e2e.sh
2880a80 
Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com>
100mik
100mik reviewed Mar 28, 2023
View reviewed changes
cli/test/e2e/env.go Outdated Show resolved Hide resolved
cli/pkg/kctrl/cmd/package/available/default_values.go Outdated Show resolved Hide resolved
cli/test/e2e/release_with_changed_spec_test.go Outdated Show resolved Hide resolved
@praveenrewar praveenrewar force-pushed the defaul-values-file-output branch 2 times, most recently from bc05d42 to b451598 Compare March 28, 2023 10:02
@praveenrewar
Include required properties in --default-values
0ad2332 
-file-output

Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com>
@praveenrewar praveenrewar changed the title Include properties with no default value in --default-values-file-output Include required properties in --default-values-file-output May 23, 2023
@praveenrewar praveenrewar requested a review from 100mik May 23, 2023 19:52
@praveenrewar
Improve errors for empty values schema
eec7886 
in package available get

Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com>
100mik
100mik reviewed May 29, 2023
View reviewed changes
Copy link
Contributor

@100mik 100mik left a comment

Choose a reason for hiding this comment

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

Some thoughts.

Since this PR has a few refactors/improvements separated by commits I was wondering if it would be worth cleaning up some of the nomenclature in default_values.go

The nit mentioned aside I think we should rename the slice of booleans (b) and even x (this seems to have different meanings in different scopes?)
k = key is the conclusion I drew , but maybe this could have a better name too?

My main concern here is legibility, took me a bit to figure out what b is in context of createDefault

cli/hack/test-e2e.sh Show resolved Hide resolved
cli/pkg/kctrl/cmd/package/available/default_values.go Show resolved Hide resolved
@praveenrewar
Copy link
Member Author

Since this PR has a few refactors/improvements separated by commits I was wondering if it would be worth cleaning up some of the nomenclature in default_values.go

I have done the refactoring in the bash script only and tried to keep the changes in the other files to a minimal. I wanted to do more refactoring in default-values.go but I feel that a separate PR would make more sense?

@100mik
Copy link
Contributor

100mik commented May 30, 2023

I think I am fine with a separate PR for this stuff
Track this somewhere maybe?

100mik
100mik approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@100mik 100mik left a comment

Choose a reason for hiding this comment

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

LGTM!

@praveenrewar praveenrewar merged commit 37fd718 into develop May 30, 2023
@praveenrewar praveenrewar deleted the defaul-values-file-output branch May 30, 2023 08:43
@praveenrewar
Copy link
Member Author

Track this somewhere maybe?

I will be creating a new PR soon.

@github-actions github-actions bot added the carvel-triage This issue has not yet been reviewed for validity label May 30, 2023
@praveenrewar praveenrewar added cli Issue for kapp-controller cli and removed carvel-triage This issue has not yet been reviewed for validity labels Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@100mik 100mik 100mik approved these changes

Assignees
No one assigned
Labels
cli Issue for kapp-controller cli
Projects
Carvel
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@praveenrewar @100mik