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(ui): visualize oci annotation #3715

Merged
merged 14 commits into from
Mar 27, 2025
Merged

feat(ui): visualize oci annotation #3715

merged 14 commits into from
Mar 27, 2025

Conversation

Marvin9
Copy link
Contributor

@Marvin9 Marvin9 commented Mar 25, 2025

head from #3676

Screenshot 2025-03-26 at 1 06 57 AM (2)

Screenshot 2025-03-26 at 3 16 01 PM (2)

hiddeco and others added 6 commits March 19, 2025 21:37
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9 Marvin9 requested a review from a team as a code owner March 25, 2025 19:50
Copy link

netlify bot commented Mar 25, 2025

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit c402fd1
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/67e5a5e1d2311a0008b066ac
😎 Deploy Preview https://deploy-preview-3715.docs.kargo.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Mar 25, 2025

Codecov Report

Attention: Patch coverage is 45.45455% with 12 lines in your changes missing coverage. Please review.

Project coverage is 21.90%. Comparing base (9e5eafa) to head (c402fd1).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
internal/image/repository_client.go 45.45% 9 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3715      +/-   ##
==========================================
+ Coverage   21.82%   21.90%   +0.08%     
==========================================
  Files         312      313       +1     
  Lines       65571    65708     +137     
==========================================
+ Hits        14311    14396      +85     
- Misses      50490    50536      +46     
- Partials      770      776       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@krancour
Copy link
Member

@Marvin9 I test drove this with arbitrary annotation foo=bar and didn't see it come through in the UI. Looking at the code, it seems this only displays two specific, well-defined annotations. Have I got that right?

If so, I am wondering if it would be better to display all the annotations that @hiddeco's changes find.

@jessesuen how were you expecting this to work?

@Marvin9
Copy link
Contributor Author

Marvin9 commented Mar 26, 2025

Correct @krancour its because those are the thing that are meaningful, all other annotation you can see it from manifest. I will update at least freight details to show more raw annotation values

@Marvin9
Copy link
Contributor Author

Marvin9 commented Mar 26, 2025

@krancour would you test this again? Now it shows all OCI annotation in freight details

@jessesuen
Copy link
Member

Sorry for commenting late but I was in agreement with Kent in that we should be able to see all the annotations.

@Marvin9
Copy link
Contributor Author

Marvin9 commented Mar 27, 2025

Sorry for commenting late but I was in agreement with Kent in that we should be able to see all the annotations.

I have updated UI to show all OCI in freight details. @krancour Please test now

@nikolay-te
Copy link

This is great! Would these be also eventually be accessible in the freight, e.g. be able to use them during promotions?

@hiddeco
Copy link
Contributor

hiddeco commented Mar 27, 2025

@nikolay-te the imageFrom() expression function will return an object with an Annotations field.

@krancour
Copy link
Member

I was in agreement with Kent in that we should be able to see all the annotations.

@jessesuen, my apologies, but we need more clarification still...

Is "all" really all just those with predefined keys?

Currently this still will not show completely arbitrary ones like foo: bar.

Comment on lines 16 to 47
export const getImageSource = (annotation: Annotation) => {
const url = annotation?.[ociAnnotationKeys.source];
const revision = annotation?.[ociAnnotationKeys.revision];

if (!revision) {
return url;
}

let baseUrl;

if (url.includes('github.com')) {
baseUrl = url
.replace(/^git@github.com:/, 'https://github.com/')
.replace(/^https?:\/\/github.com\//, 'https://github.com/')
.replace(/\.git$/, '');
return `${baseUrl}/commit/${revision}`;
} else if (url.includes('gitlab.com')) {
baseUrl = url
.replace(/^git@gitlab.com:/, 'https://gitlab.com/')
.replace(/^https?:\/\/gitlab.com\//, 'https://gitlab.com/')
.replace(/\.git$/, '');
return `${baseUrl}/-/commit/${revision}`;
} else if (url.includes('bitbucket.org')) {
baseUrl = url
.replace(/^git@bitbucket.org:/, 'https://bitbucket.org/')
.replace(/^https?:\/\/bitbucket.org\//, 'https://bitbucket.org/')
.replace(/\.git$/, '');
return `${baseUrl}/commits/${revision}`;
}

return url;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

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 think its fine

Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this to utils.ts or add utils as a suffix.

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 would add suffix. I learnt from @krancour in backend that util name is code smell and that kind of stuck to me because truly its hard to know what it contains and also hard to find the file by function name.

@jessesuen
Copy link
Member

jessesuen commented Mar 27, 2025

Currently this still will not show completely arbitrary ones like foo: bar.

I meant all as in any annotation, including foo: bar. I suspect there would be use cases to see annotations outside the image standard, especially when the OCI artifact is not container image related (e.g. wasm, yaml manifests). If we think seeing too many annotations would be an issue in the future, we could always have a toggle/preference to show only "standard" annotations.

@krancour
Copy link
Member

Thank you for the clarification @jessesuen!

Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Marvin9 added 5 commits March 28, 2025 00:35
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9 Marvin9 enabled auto-merge March 27, 2025 19:27
@Marvin9 Marvin9 added this pull request to the merge queue Mar 27, 2025
Merged via the queue into main with commit a5b6ada Mar 27, 2025
17 checks passed
@Marvin9 Marvin9 deleted the Marvin9/oci-annotations-ui branch March 27, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants