Skip to content

[runx] Add runx validate, lockfile resolve #1542

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
Merged
merged 1 commit into from
Oct 9, 2023
Merged

[runx] Add runx validate, lockfile resolve #1542

merged 1 commit into from
Oct 9, 2023

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Oct 8, 2023 •
edited
Loading

Summary

A few major changes to runx:

  • Updates newest runx version to allow for github token, validation, etc.
  • lockfile can now resolve runx packages. Exact versions are saved in lockfile and `install installs deterministic versions according to lockfile information.
  • devpkg can now validate runx packages

How was it tested?

devbox add runx:golangci/golangci-lint@latest
devbox add runx:golangci/[email protected]
devbox add runx:golangci/golangci-lint
devbox rm runx:golangci/golangci-lint

Inspected devbox.json and devbox.lock in between every command.

@mikeland73 mikeland73 requested review from gcurtis and savil October 8, 2023 22:47
@savil
Copy link
Collaborator

savil commented Oct 9, 2023

Note: While implementing changes I discovered a few API issues in runx that can be improved. Specifically:

this seems to have gotten dropped

savil
savil approved these changes Oct 9, 2023
View reviewed changes
Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

nice!

internal/devpkg/validation.go
Comment on lines +11 to +13
func (p *Package) ValidateExists(ctx context.Context) (bool, error) {
if p.IsRunX() {
// TODO implement runx validation
return true, nil
_, err := p.lockfile.Resolve(p.Raw)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I notice that ctx is unused, but I think you were trying to add it to the Resolve function signature but gave up b/c there are way too many callsites?

If so, no problem. We can complete that TODO in some follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a bunch. I wanted to prevent this PR from growing more.

@mikeland73
Copy link
Contributor Author

Note: While implementing changes I discovered a few API issues in runx that can be improved. Specifically:

this seems to have gotten dropped

I was gonna write something but then decided against it.

@mikeland73 mikeland73 merged commit 63cb177 into main Oct 9, 2023
@mikeland73 mikeland73 deleted the landau/runx-todos branch October 9, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@savil savil savil approved these changes

@gcurtis gcurtis Awaiting requested review from gcurtis

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikeland73 @savil