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

treewide: change runtime-handler naming scheme #772

Merged
merged 2 commits into from
Aug 2, 2024
Merged

treewide: change runtime-handler naming scheme #772

merged 2 commits into from
Aug 2, 2024

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Jul 30, 2024 •
edited
Loading

The Contrast runtime handlers are now named in the format contrast-cc-<platform>-<hash>, where hash is the hash of the relevant runtime components for platform and platform is the lowercase variant of the deployed platform.

@msanft msanft added the no changelog PRs not listed in the release notes label Jul 30, 2024
@msanft msanft force-pushed the msanft/runtime-handler-naming branch 2 times, most recently from e733015 to 41155e6 Compare July 30, 2024 15:25
@msanft msanft requested review from burgerdev and Freax13 July 30, 2024 15:25
@msanft msanft marked this pull request as ready for review July 30, 2024 15:25
@msanft msanft requested a review from katexochen as a code owner July 30, 2024 15:25
Freax13
Freax13 reviewed Jul 31, 2024
View reviewed changes
go.mod Outdated Show resolved Hide resolved
node-installer/internal/constants/constants.go Outdated Show resolved Hide resolved
internal/constants/constants.go Outdated Show resolved Hide resolved
node-installer/internal/constants/constants.go Outdated Show resolved Hide resolved
@msanft msanft force-pushed the msanft/runtime-handler-naming branch from 41155e6 to e7e5748 Compare July 31, 2024 06:08
@katexochen
Copy link
Member

The Contrast runtime handlers are now named in the format contrast-cc-<version>-<platform>, where version and platform are lowercase variants of the Contrast version

Why is this placing the version in the runtime handler name and not the hash of the runtime files?

@msanft msanft marked this pull request as draft July 31, 2024 07:21
@msanft msanft force-pushed the msanft/runtime-handler-naming branch from fcaf780 to e7e5748 Compare July 31, 2024 07:22
@Freax13 Freax13 mentioned this pull request Jul 31, 2024
Merged
@Freax13
Copy link
Contributor

Freax13 commented Jul 31, 2024 •
edited
Loading

The documentation needs to be updated to reflect the new name:

contrast/docs/docs/examples/emojivoto.md

Line 79 in 319bd50

A [runtime class](https://docs.edgeless.systems/contrast/components/runtime) `contrast-cc-<VERSIONHASH>`

@msanft msanft force-pushed the msanft/runtime-handler-naming branch 4 times, most recently from a24219d to 6e40ff8 Compare August 1, 2024 14:59
@msanft msanft changed the title Change runtime-handler naming scheme treewide: change runtime-handler naming scheme Aug 1, 2024
@msanft msanft force-pushed the msanft/runtime-handler-naming branch from 6e40ff8 to 80685d3 Compare August 1, 2024 15:27
@msanft msanft marked this pull request as ready for review August 1, 2024 15:27
@msanft msanft requested review from burgerdev and Freax13 August 1, 2024 15:27
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 1, 2024 •
edited
Loading

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-08-02 12:48 UTC

burgerdev
burgerdev reviewed Aug 1, 2024
View reviewed changes
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
internal/manifest/runtimehandler_test.go Outdated Show resolved Hide resolved
nodeinstaller/internal/constants/constants.go Outdated Show resolved Hide resolved
Freax13
Freax13 reviewed Aug 2, 2024
View reviewed changes
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
packages/by-name/contrast/package.nix Outdated Show resolved Hide resolved
@msanft msanft force-pushed the msanft/runtime-handler-naming branch from 80685d3 to 81b98be Compare August 2, 2024 06:44
@msanft msanft requested review from Freax13 and burgerdev August 2, 2024 06:44
burgerdev
burgerdev approved these changes Aug 2, 2024
View reviewed changes
Copy link
Contributor

@burgerdev burgerdev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Freax13
Copy link
Contributor

Freax13 commented Aug 2, 2024

We still need to change this path here:

contrast/packages/by-name/kata/contrast-node-installer-image/package.nix

Lines 29 to 31 in 81b98be

launch-digest = lib.removeSuffix "\n" (
builtins.readFile "${kata.runtime-class-files}/launch-digest.hex"
);

katexochen
katexochen requested changes Aug 2, 2024
View reviewed changes
.../by-name/microsoft/genpolicy/0004-genpolicy-regex-check-contrast-specific-layer-src-pr.patch Outdated Show resolved Hide resolved
packages/by-name/microsoft/runtime-class-files/package.nix Outdated Show resolved Hide resolved
nodeinstaller/node-installer.go Outdated Show resolved Hide resolved
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
internal/manifest/referencevalues.go Outdated Show resolved Hide resolved
packages/by-name/kata/contrast-node-installer-image/package.nix Outdated Show resolved Hide resolved
@Freax13
Copy link
Contributor

Freax13 commented Aug 2, 2024

DefaultPlatformHandler and by extension Manifest.RuntimeHandler are no longer needed.

@msanft msanft force-pushed the msanft/runtime-handler-naming branch from 81b98be to aade31a Compare August 2, 2024 09:00
@msanft msanft requested a review from katexochen August 2, 2024 09:01
@msanft msanft force-pushed the msanft/runtime-handler-naming branch from aade31a to 265e7d4 Compare August 2, 2024 09:03
@msanft msanft force-pushed the msanft/runtime-handler-naming branch 2 times, most recently from b5e3827 to 8f24add Compare August 2, 2024 09:46
@msanft msanft requested review from Freax13 and burgerdev August 2, 2024 09:46
Freax13
Freax13 reviewed Aug 2, 2024
View reviewed changes
nodeinstaller/node-installer_test.go Outdated Show resolved Hide resolved
nodeinstaller/node-installer.go Outdated Show resolved Hide resolved
packages/by-name/contrast/package.nix Outdated Show resolved Hide resolved
@msanft msanft force-pushed the msanft/runtime-handler-naming branch from 8f24add to 2cc8939 Compare August 2, 2024 12:26
@msanft msanft requested a review from katexochen August 2, 2024 12:26
msanft added 2 commits August 2, 2024 14:29
@msanft
treewide: change runtime-handler naming scheme
1335e7b 
The Contrast runtime handlers are now named in the format `contrast-cc--<platform>-<hash>`, where `<hash>` is the hash of the relevant runtime components for platform and `<platform>` is the lowercase variant of the deployed platform.
@msanft
docs: update runtime-class references to new naming scheme
0eb46f1
@msanft msanft force-pushed the msanft/runtime-handler-naming branch from 2cc8939 to 0eb46f1 Compare August 2, 2024 12:29
@msanft msanft merged commit 0697c37 into main Aug 2, 2024
12 checks passed
@msanft msanft deleted the msanft/runtime-handler-naming branch August 2, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katexochen katexochen katexochen approved these changes

@Freax13 Freax13 Freax13 approved these changes

@burgerdev burgerdev Awaiting requested review from burgerdev burgerdev is a code owner

Assignees
No one assigned
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@msanft @katexochen @Freax13 @burgerdev