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

Add option --allow-change-package-name in the package installed update operation #1671

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add option --allow-change-package-name in the package installed update operation #1671

wants to merge 1 commit into from

Conversation

luwangVMW
Copy link

Add option allow-change-package-name in the package installed update operation

this option allows user to change the package name for the packageinstalled CR

What this PR does / why we need it:

We need to change the package name for the installed package. However the current code blocks it .

Which issue(s) this PR fixes:

Fixes #1651

Does this PR introduce a user-facing change?

this option --allow-change-package-name allows user to change the package name when calling package installed update operation

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.:

AkbaraliShaikh
AkbaraliShaikh reviewed Jan 17, 2025
View reviewed changes
Copy link

@AkbaraliShaikh AkbaraliShaikh left a comment

Choose a reason for hiding this comment

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

Thank you for the PR,

Have the changes in the PR been tested?

cli/pkg/kctrl/cmd/package/installed/create_or_update.go Outdated Show resolved Hide resolved
cli/pkg/kctrl/cmd/package/installed/create_or_update.go Outdated Show resolved Hide resolved
@luwangVMW luwangVMW force-pushed the option_update_package_name branch from a66d0ac to 1d792a9 Compare January 22, 2025 02:05
@luwangVMW
Add option allow-package-name-change in the package installed update …
12f84d7 
…operation

this option allows user to change the package name for the packageinstalled CR

Signed-off-by: luwang <[email protected]>
@luwangVMW luwangVMW force-pushed the option_update_package_name branch from 1d792a9 to 12f84d7 Compare January 22, 2025 02:10
@luwangVMW
Copy link
Author

luwangVMW commented Jan 22, 2025 •
edited
Loading

Thank you for the PR,

Have the changes in the PR been tested?

Yes. Please check the output below

root@corgi-jumper:~/telegraf# kctrl  package installed list -n telegraf-installed
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

Installed packages in namespace 'telegraf-installed'

Name      Package Name             Package Version        Status  
telegraf  telegraf.test1.com  1.32.1+test.1  Reconcile succeeded  

Succeeded

root@corgi-jumper:~/carvel/kapp-controller/cli# kctrl package installed update -i telegraf  -n telegraf-installed -p telegraf.test2.com -v 1.32.1+test.2  --values-file ~/telegraf/telegraf-data-values.yaml  **--allow-package-name-change** 
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

3:56:07AM: Pausing reconciliation for package installation 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Updating secret 'telegraf-telegraf-installed-values'
3:56:08AM: Updating package install for 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Resuming reconciliation for package installation 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Waiting for PackageInstall reconciliation for 'telegraf'
3:56:08AM: Waiting for generation 5 to be observed 
3:55:44AM: Deploy succeeded (24s ago)

Succeeded

root@corgi-jumper:~/telegraf# kctrl  package installed list -n telegraf-installed
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

Installed packages in namespace 'telegraf-installed'

Name      Package Name             Package Version        Status  
telegraf  telegraf.test2.com  1.32.1+test.2 Reconcile succeeded  

Succeeded

@mamachanko
Copy link
Contributor

mamachanko commented Jan 22, 2025 •
edited
Loading

Thank you for the PR,
Have the changes in the PR been tested?

Yes. Please check the output below

root@corgi-jumper:~/telegraf# kctrl  package installed list -n telegraf-installed
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

Installed packages in namespace 'telegraf-installed'

Name      Package Name             Package Version        Status  
telegraf  telegraf.test1.com  1.32.1+test.1  Reconcile succeeded  

Succeeded

root@corgi-jumper:~/carvel/kapp-controller/cli# kctrl package installed update -i telegraf  -n telegraf-installed -p telegraf.test2.com -v 1.32.1+test.2  --values-file ~/telegraf/telegraf-data-values.yaml  **--allow-package-name-change** 
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

3:56:07AM: Pausing reconciliation for package installation 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Updating secret 'telegraf-telegraf-installed-values'
3:56:08AM: Updating package install for 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Resuming reconciliation for package installation 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Waiting for PackageInstall reconciliation for 'telegraf'
3:56:08AM: Waiting for generation 5 to be observed 
3:55:44AM: Deploy succeeded (24s ago)

Succeeded

root@corgi-jumper:~/telegraf# kctrl  package installed list -n telegraf-installed
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

Installed packages in namespace 'telegraf-installed'

Name      Package Name             Package Version        Status  
telegraf  telegraf.test2.com  1.32.1+test.2 Reconcile succeeded  

Succeeded

[issue]: we should have coverage for this in test/e2e/kappcontroller or cli/test/e2e, don't you think?

@luwangVMW
Copy link
Author

Thank you for the PR,
Have the changes in the PR been tested?

Yes. Please check the output below

root@corgi-jumper:~/telegraf# kctrl  package installed list -n telegraf-installed
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

Installed packages in namespace 'telegraf-installed'

Name      Package Name             Package Version        Status  
telegraf  telegraf.test1.com  1.32.1+test.1  Reconcile succeeded  

Succeeded

root@corgi-jumper:~/carvel/kapp-controller/cli# kctrl package installed update -i telegraf  -n telegraf-installed -p telegraf.test2.com -v 1.32.1+test.2  --values-file ~/telegraf/telegraf-data-values.yaml  **--allow-package-name-change** 
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

3:56:07AM: Pausing reconciliation for package installation 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Updating secret 'telegraf-telegraf-installed-values'
3:56:08AM: Updating package install for 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Resuming reconciliation for package installation 'telegraf' in namespace 'telegraf-installed'
3:56:08AM: Waiting for PackageInstall reconciliation for 'telegraf'
3:56:08AM: Waiting for generation 5 to be observed 
3:55:44AM: Deploy succeeded (24s ago)

Succeeded

root@corgi-jumper:~/telegraf# kctrl  package installed list -n telegraf-installed
Target cluster 'https://192.168.0.3:6443' (nodes: wl-antrea-7h8lf-pftgk, 5+)

Installed packages in namespace 'telegraf-installed'

Name      Package Name             Package Version        Status  
telegraf  telegraf.test2.com  1.32.1+test.2 Reconcile succeeded  

Succeeded

[issue]: we should have coverage for this in test/e2e/kappcontroller or cli/test/e2e, don't you think?

@mamachanko , thanks for the comments. I have less knowledge about the testing. Please advice which folder I need to add new test for my changes.

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

@AkbaraliShaikh AkbaraliShaikh AkbaraliShaikh left review comments

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

Assignees
No one assigned
Labels
None yet
Projects
Carvel
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

to support --allow-change-package-name (or similar) in the kctrl pacakge installed update command
3 participants
@luwangVMW @mamachanko @AkbaraliShaikh