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

Allow to update the packageinstall with a different packageName #1671

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Allow to update the packageinstall with a different packageName #1671

merged 1 commit into from
Feb 12, 2025

Conversation

luwangVMW
Copy link
Contributor

@luwangVMW luwangVMW commented Jan 15, 2025 •
edited
Loading

Allow to update the packageinstall with a difference packageName

  1. Prompt user to confirm
  2. Add e2e test for this operation

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
detail check https://kubernetes.slack.com/archives/CH8KCCKA5/p1736907725646329

Does this PR introduce a user-facing change?

NONE

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?

@luwangVMW luwangVMW force-pushed the option_update_package_name branch 2 times, most recently from 1d792a9 to 12f84d7 Compare January 22, 2025 02:10
@luwangVMW
Copy link
Contributor 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
Contributor 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.

@luwangVMW luwangVMW force-pushed the option_update_package_name branch from 12f84d7 to 65fe27a Compare January 26, 2025 06:27
@luwangVMW luwangVMW changed the title Add option --allow-change-package-name in the package installed update operation Allow to update packageinstall with changing packageName Jan 26, 2025
@luwangVMW luwangVMW force-pushed the option_update_package_name branch from 65fe27a to 1396414 Compare January 26, 2025 06:49
@luwangVMW luwangVMW force-pushed the option_update_package_name branch from 1396414 to 69902fd Compare February 6, 2025 02:38
@luwangVMW
Copy link
Contributor Author

@AkbaraliShaikh I did not succeed to run e2e test in my own desktop. Would you mind help approve the 5 workflows to run the e2e test in the github pipeline?

@luwangVMW luwangVMW force-pushed the option_update_package_name branch from 69902fd to da4ba4b Compare February 6, 2025 07:04
@luwangVMW luwangVMW changed the title Allow to update packageinstall with changing packageName Allow to update the packageinstall with a difference packageName Feb 7, 2025
@luwangVMW
Allow to update the packageinstall with a difference packageName
859905e 
1. Prompt user to confirm
2. Add e2e test for this operation

Signed-off-by: luwang <[email protected]>
@luwangVMW luwangVMW force-pushed the option_update_package_name branch from da4ba4b to 859905e Compare February 10, 2025 06:58
@luwangVMW luwangVMW changed the title Allow to update the packageinstall with a difference packageName Allow to update the packageinstall with a different packageName Feb 11, 2025
@100mik 100mik merged commit d9a094c into carvel-dev:develop Feb 12, 2025
6 checks passed
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

@100mik 100mik 100mik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Carvel
Archived in project
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
4 participants
@luwangVMW @mamachanko @AkbaraliShaikh @100mik