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

feat: implement outdated command #2497

Merged
merged 2 commits into from
Feb 5, 2025
Merged

feat: implement outdated command #2497

merged 2 commits into from
Feb 5, 2025

Conversation

guerinoni
Copy link
Contributor

@guerinoni guerinoni commented Jan 31, 2025

Summary

This allow to present all new versions available for the pkg installed. It is a lot useful because otherwise you need to check this manually.

How was it tested?

In a different repository, this is the output.

Screenshot 2025-01-31 at 17 54 25

This allow to present all new versions available for the pkg installed.
It is a lot useful because otherwise you need to check this manually.
@savil
Copy link
Collaborator

savil commented Jan 31, 2025

thanks for sending a PR!
@Lagoja adding you to see if you have thoughts on how to expose this functionality. My thoughts are that we generally try to keep the top-level command list small. Otherwise doing devbox --help can get overwhelmingly long. Since this has to do with updating packages, would it make sense to do devbox update ls-outdated or similar?

@guerinoni
Copy link
Contributor Author

For sure it is something we can move under another menu, no problem!

@Lagoja
Copy link
Contributor

Lagoja commented Jan 31, 2025

I think this might be best to move under devbox list? I think something like devbox list --outdated probably fits well with the current top level commands.

@guerinoni
Copy link
Contributor Author

@savil @Lagoja updated code to be under list --outdated

@savil savil requested review from gcurtis and mikeland73 February 3, 2025 21:41
Comment on lines 64 to 65
result, err := searcher.Client().Search(ctx, pkg.CanonicalName())
if err != nil {
return nil, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gcurtis does this reliably give us the latest version of the package?

For example, when I run it with go I get:

 devbox ls --outdated
The following packages can be updated:
 * go                             1.23.4 -> 1.24rc2

which doesn't seem to be what we want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this is correct, is the latest version available in the store :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

But its a release-candidate. In devbox update we wouldn't actually update you to that version if you had go@latest

}
}

return outdatedPackages, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

@guerinoni I think we need this function to be rewritten as so. I created this patch via git diff --patch, so you could try applying it: https://gist.github.com/savil/7a55e959c5eb5c5e240f57dedd5fd7ad

copying here for clarity:

type UpdateVersion struct {
	Current string
	Latest  string
}

// Outdated returns a map of package names to their available latest version.
func (d *Devbox) Outdated(ctx context.Context) (map[string]UpdateVersion, error) {
	ctx, task := trace.NewTask(ctx, "devboxOutdated")
	defer task.End()

	lockfile := d.Lockfile()
	outdatedPackages := map[string]UpdateVersion{}

	for _, pkg := range d.AllPackages() {
		// For non-devbox packages, like flakes or runx, we can skip for now
		if !pkg.IsDevboxPackage {
			continue
		}

		lockPackage, err := lockfile.FetchResolvedPackage(pkg.Versioned())
		if err != nil {
			return nil, errors.Wrap(err, "failed to fetch resolved package")
		}
		existingLockPackage := lockfile.Packages[pkg.Raw]
		if lockPackage.Version == existingLockPackage.Version {
			continue
		}

		outdatedPackages[pkg.Versioned()] = UpdateVersion{Current: existingLockPackage.Version, Latest: lockPackage.Version}
	}

	return outdatedPackages, nil
}

wdyt @gcurtis @mikeland73 ?

The benefit of this is that it also works for @latest. For example, when I run this on the devbox repo itself, I see:

$ devbox run build
$  dist/devbox ls --outdated
The following packages can be updated:
 * go@latest                      1.23.0 -> 1.23.4
 * runx:golangci/golangci-lint@latest v1.60.2 -> v1.63.4

I don't think the runx one should be getting printed, but its cause we return true for IsDevboxPackage for runx packages!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I aligned with this patch :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not runx? The versions are correct. Ideally we could also do it for flakes as well but our current flake update mechanism is a bit broken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh good point, I forgot runx updating works properly. So all good

Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

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

minor nits, but this LGTM

defer task.End()

lockfile := d.Lockfile()
outdatedPackages := map[string]UpdateVersion{}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, would prefer to use a slice over map. We don't need random access or deduplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gcurtis may not like this but we could just return []lo.Tuple2[*lockfile.Package, *lockfile.Package]

that way we don't have to create a new struct at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm I don't mind the struct. Its nice to have the names

Copy link
Collaborator

Choose a reason for hiding this comment

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

The map is just a convenience for the name. Otherwise you'd have to add a Name field to the struct. That works too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How you prefer guys, here map doesn't create any performance problems, it is just a matter of some seconds to run the command, I chose readability over little optimization

}
}

return outdatedPackages, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not runx? The versions are correct. Ideally we could also do it for flakes as well but our current flake update mechanism is a bit broken.

@savil savil merged commit ff381aa into jetify-com:main Feb 5, 2025
29 checks passed
@guerinoni guerinoni deleted the outdated branch February 5, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants