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

feat: add dry-run flag for sync and apply #1050

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat: add dry-run flag for sync and apply #1050

wants to merge 8 commits into from

Conversation

yxxhero
Copy link
Member

@yxxhero yxxhero commented Sep 30, 2023

see: #1024

@yxxhero yxxhero marked this pull request as ready for review September 30, 2023 10:38
@xiaomudk
Copy link
Contributor

@yxxhero Thanks for your hard work, it would be better if you add some test cases.

@yxxhero
Copy link
Member Author

yxxhero commented Oct 12, 2023

@yxxhero Thanks for your hard work, it would be better if you add some test cases.

got it.

@stale
Copy link

stale bot commented Oct 28, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Oct 28, 2023
@yxxhero yxxhero removed the wontfix This will not be worked on label Oct 28, 2023
@felipecrs
Copy link
Contributor

This would be an useful feature.

@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch from 59bb9a8 to 9b2e9b5 Compare October 29, 2023 02:41
@stale Stale
Copy link

stale bot commented Nov 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 12, 2023
@yxxhero yxxhero removed the wontfix This will not be worked on label Nov 12, 2023
@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch from 9b2e9b5 to 44eba38 Compare November 12, 2023 12:24
@stale Stale
Copy link

stale bot commented Nov 26, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 26, 2023
@yxxhero yxxhero removed the wontfix This will not be worked on label Nov 26, 2023
@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch from 44eba38 to 7f77237 Compare November 26, 2023 23:55
@balagurrr
Copy link

yep, it's very useful feature

hoangelos
hoangelos reviewed Dec 14, 2023
View reviewed changes
Copy link

@hoangelos hoangelos left a comment

Choose a reason for hiding this comment

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

If you can't finish the unitest of this, I'd do it, because it's creating some big problems not having access to the dry-run.

cmd/apply.go Outdated
@@ -68,6 +68,7 @@ func NewApplyCmd(globalCfg *config.GlobalImpl) *cobra.Command {
f.StringVar(&applyOptions.PostRenderer, "post-renderer", "", `pass --post-renderer to "helm template" or "helm upgrade --install"`)
f.StringArrayVar(&applyOptions.PostRendererArgs, "post-renderer-args", nil, `pass --post-renderer-args to "helm template" or "helm upgrade --install"`)
f.StringVar(&applyOptions.Cascade, "cascade", "", "pass cascade to helm exec, default: background")
f.StringVar(&applyOptions.DryRyn, "dry-run", "", "pass dry-run to helm exec")
Copy link

Choose a reason for hiding this comment

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

Isn't this a typo?

cmd/sync.go Outdated
@@ -47,6 +47,7 @@ func NewSyncCmd(globalCfg *config.GlobalImpl) *cobra.Command {
f.StringVar(&syncOptions.PostRenderer, "post-renderer", "", `pass --post-renderer to "helm template" or "helm upgrade --install"`)
f.StringArrayVar(&syncOptions.PostRendererArgs, "post-renderer-args", nil, `pass --post-renderer-args to "helm template" or "helm upgrade --install"`)
f.StringVar(&syncOptions.Cascade, "cascade", "", "pass cascade to helm exec, default: background")
f.StringVar(&syncOptions.DryRyn, "dry-run", "", "pass dry-run to helm exec")
Copy link

Choose a reason for hiding this comment

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

Isn't DryRyn a typo?

pkg/config/sync.go Outdated
@@ -32,6 +32,8 @@ type SyncOptions struct {
PostRendererArgs []string
// Cascade '--cascade' to helmv3 delete, available values: background, foreground, or orphan, default: background
Cascade string
// DryRun is for helm dry-run flag
DryRyn string
Copy link

Choose a reason for hiding this comment

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

Typo

pkg/config/sync.go Outdated

// DryRun returns dry-run flag
func (t *SyncImpl) DryRun() string {
return t.SyncOptions.DryRyn
Copy link

Choose a reason for hiding this comment

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

Typo

@yxxhero
Copy link
Member Author

yxxhero commented Dec 14, 2023

If you can't finish the unitest of this, I'd do it, because it's creating some big problems not having access to the dry-run.

Yeah. Please feel free to do that.

@yxxhero
Copy link
Member Author

yxxhero commented Dec 16, 2023

@hoangelos ping

@hoangelos
Copy link

hoangelos commented Dec 18, 2023 •
edited
Loading

@yxxhero I can't push the UnitTest to this branch. It's saying I don't have rights to do that

MREMCDBAFC2C:helmfile peter.halliday$ git push upstream add-dry-run-flag-for-sync-and-apply
ERROR: Permission to helmfile/helmfile.git denied to hoangelos.
fatal: Could not read from remote repository.

@yxxhero
Copy link
Member Author

yxxhero commented Dec 18, 2023

@hoangelos oh. you can create a new PR for this.

@hoangelos
Copy link

@yxxhero I've doing a separate PR to push into the branch this PR is on.

#1235

@yxxhero yxxhero closed this Dec 18, 2023
@yxxhero yxxhero reopened this Dec 18, 2023
@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch from b06eff8 to 9135c51 Compare December 22, 2023 23:20
@hoangelos
Copy link

Looks like it’s not Boolean. But the argument needs to be optional. And I’ll make sure it’s only supported for after this version.

@hoangelos
Copy link

@yxxhero have a PR to add unit tests, restriction on version, and default for if nothing is passed:

#1265

yxxhero
yxxhero commented Jan 3, 2024
View reviewed changes
pkg/state/helmx_test.go Outdated Show resolved Hide resolved
yxxhero
yxxhero commented Jan 3, 2024
View reviewed changes
pkg/state/helmx_test.go Outdated Show resolved Hide resolved
@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch from 73b60f0 to 7b8468c Compare February 10, 2024 04:27
@yxxhero yxxhero marked this pull request as ready for review February 10, 2024 04:27
@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch 7 times, most recently from 2fba764 to fb7c54e Compare February 13, 2024 10:57
@felipecrs
Copy link
Contributor

felipecrs commented Mar 1, 2024 •
edited
Loading

@yxxhero just a question, what would happen with hooks in your implementation? Would they still execute in dry run mode?

@yxxhero
Copy link
Member Author

yxxhero commented Mar 2, 2024

@felipecrs good question. I need to think more.

@felipecrs
Copy link
Contributor

Some ideas when dry run is on:

  1. Don't run any hooks
  2. Don't run release-level hooks, but still run global hooks
  3. Run hooks, but provide some template variable so that hooks can identify whether they are running in dry run mode or not

@yxxhero
Copy link
Member Author

yxxhero commented Mar 2, 2024

@felipecrs thanks so much. good ideas.

@yxxhero yxxhero mentioned this pull request May 25, 2024
Open
@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch from fb7c54e to 3b73c6f Compare May 25, 2024 14:39
@yxxhero
Copy link
Member Author

yxxhero commented May 28, 2024

Some ideas when dry run is on:

  1. Don't run any hooks
  2. Don't run release-level hooks, but still run global hooks
  3. Run hooks, but provide some template variable so that hooks can identify whether they are running in dry run mode or not

I will follow the point 3. thanks so much.

@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch from 3b73c6f to 6752cfe Compare May 28, 2024 13:36
yxxhero and others added 8 commits June 2, 2024 15:03
@yxxhero
fix conflicts
279d7de 
Signed-off-by: yxxhero <[email protected]>
@hoangelos @yxxhero
Fix typos and ask unittest (#1235)
4b3ad24 
Signed-off-by: yxxhero <[email protected]>
@yxxhero
fix tests
b5095c0 
Signed-off-by: yxxhero <[email protected]>
@hoangelos @yxxhero
Restrict version of helm, and default dryRun. (#1265)
ac25cc5 
Signed-off-by: yxxhero <[email protected]>
@yxxhero
fix issues
c710e2b 
Signed-off-by: yxxhero <[email protected]>
@yxxhero
fix issues
cc3202e 
Signed-off-by: yxxhero <[email protected]>
@yxxhero
fix more issue
61b4199 
Signed-off-by: yxxhero <[email protected]>
@yxxhero
add runinfo
2ebdf9a 
Signed-off-by: yxxhero <[email protected]>
@yxxhero yxxhero force-pushed the add-dry-run-flag-for-sync-and-apply branch from bac765c to 2ebdf9a Compare June 2, 2024 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaomudk xiaomudk xiaomudk left review comments

@hoangelos hoangelos hoangelos left review comments

@mumoshu mumoshu Awaiting requested review from mumoshu mumoshu is a code owner

@itscaro itscaro Awaiting requested review from itscaro itscaro is a code owner

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
5 participants
@yxxhero @xiaomudk @felipecrs @balagurrr @hoangelos