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

Verify fastly crate version during compute build. #67

Merged
merged 6 commits into from
May 12, 2020

Conversation

phamann
Copy link
Member

@phamann phamann commented May 11, 2020

TL;DR

Verifies that the fastly crate exists as a dependency and is up-to-date during fastly compute build. This allows us to ensure that the package program is compatible with the edge runtime environment at compilation time.

@phamann phamann added the enhancement New feature or request label May 11, 2020
@phamann phamann force-pushed the phamann/verify-fastly-crate-version branch 2 times, most recently from b034c82 to 4f54b8f Compare May 11, 2020 16:19
Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

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

This looks really nice! Unfortunately cargo update is not gonna be sufficient for our needs here 😞

pkg/compute/rust.go Show resolved Hide resolved
pkg/compute/rust.go Show resolved Hide resolved
@phamann phamann requested a review from acfoltzer May 11, 2020 18:58
@phamann phamann force-pushed the phamann/verify-fastly-crate-version branch from c02afd9 to 58ed8bd Compare May 11, 2020 18:59
Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

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

Looks good! I can't really comment on the Go style but the semantics look right to me 😂

@phamann phamann force-pushed the phamann/verify-fastly-crate-version branch from 2f4dd6c to 16b328c Compare May 12, 2020 11:01
@phamann
Copy link
Member Author

phamann commented May 12, 2020

@acfoltzer and I discussed this PR on slack, we realised that prompting a user to add fastly = "^0.3.2" to their Cargo.toml wasn't a sufficient fix as the lock file also need updating before the verification runs again. Therefore, we've now added a dual step remediation message in these cases. However if their current version is within the semver minor range of the latest, then only a cargo update -p fastly is needed, the logic now caters for these edge cases and serves the appropriate message. I've also added a more robust test suit to ensure this new logic is covered.

@peterbourgon When/If you have some spare time, I'd appreciate a quick review on this. Danke!

Copy link
Contributor

@peterbourgon peterbourgon left a comment

Choose a reason for hiding this comment

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

Some nits but I grok it 👍

pkg/compute/rust.go Outdated Show resolved Hide resolved
pkg/compute/rust.go Outdated Show resolved Hide resolved
pkg/compute/rust.go Show resolved Hide resolved
),
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a request for change, but I'm surprised this level of semantic version "parsing" is required by Verify...

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha me too 😆

pkg/compute/rust.go Outdated Show resolved Hide resolved
Co-authored-by: Peter Bourgon <peterbourgon@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants