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.

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

2404: added update command #2412

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

slimus
Copy link
Collaborator

@slimus slimus commented Dec 30, 2023

Added command to update binary.

Need some help to test it with different operation systems

  • Update Readme
  • Test macos
  • Test windows
  • Test linux
  • Add unit tests

Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@slimus First off, Thank you so much Alex for your good work on this!
This is a great convenience and something that has been requested in the past.

I think there are several ideas to explore here.
Dealing with users env is always dicey as we enter the I drather have X scene.

I'll propose we ship this as such with a few TLCs at first, gather users feedback and then rinse/repeat on enhancements.
I think minimally we could put some lipstick on it and offer just an upgrade functionality to the latest rev.
We do need to decide at this juncture our upgrade/downgrade strategy ie hard download or download+symlink or ??


func selfUpdate() *cobra.Command {
command := cobra.Command{
Use: "selfupdate",
Copy link
Owner

Choose a reason for hiding this comment

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

To me selfUpdate has the connotation of updating while the app is running.
Perhaps update or upgrade might be more appropriate?
Also what should happen if a user has current k9s session(s) up and running while updating the binary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Soooo, this is where holywar starts :):

  1. Homebrew uses update command to update brew, and upgrade to update packages.
  2. Composer uses self-update
  3. Helm doesn't have selfupdate command, but they use upgrade cli to update helm releases.
  4. Pip uses pip install --upgrade pip to update pip itself.
  5. ....

So, this is really hard to pick one command, but at the same time - I don't want to have multiple commands to achieve only one simple result :)

I'm okay to change this command to update.

Use: "selfupdate",
Short: "Update k9s binary",
Long: "Update k9s binary",
Run: func(cmd *cobra.Command, args []string) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should define additional flags here:

  1. The default aka upgrade to the latest rev
  2. Update to a given version
  3. List the last x releases and dates
  4. Rollback to previous (default) or given version

Perhaps leveraging symlinks here could be interesting?? ie keep the last x on disk and just swap the link if present??
Given my release track record that could be handy... ;)
In this case we could offer a purge option to axe previous revs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is great improvements. I forgot about rollbacks at all! I will add these options.

}

if latest.LessOrEqual(version) {
log.Info().Msgf("Current version (%s) is the latest", version)
Copy link
Owner

Choose a reason for hiding this comment

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

Likely users of this command won't be looking at logs. Best to output the results to the console stdout or stderr.
Also thinking it might be good idea to match other k9s command look and feel ie banner + colorized output.

BONUS: leverage a progress bar (https://github.com/schollz/progressbar) or other to provide feedback

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great catch! Yes, I'll change it to use stdout instead of logs and try to add progressbar :)

}

func updateApp(latest *selfupdate.Release) error {
exe, err := os.Executable()
Copy link
Owner

Choose a reason for hiding this comment

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

What if this is a link?
Also what if this process borks and we leave things in no mans land?
Perhaps best to download the requested rev in a tmp location and if we are cool size/checksum move to the final resting place ie real or link?
Also not sure if we get this for free or not but we should validate checksum to make sure we got real mcCoy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's why I asked to help me to test all scenarios. I'll test links and let you know later.

About checksums - selfupdate library should check sums, but again, I'll test it and let you know later.

@slimus slimus marked this pull request as draft January 9, 2024 01:21
@slimus slimus changed the title 2404: added selfupdate command 2404: added update command Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants