-
Notifications
You must be signed in to change notification settings - Fork 178
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
Add s3 storage artifact route and ui integration of it #2827
Add s3 storage artifact route and ui integration of it #2827
Conversation
keeping this as WIP until security review is done. I will also add tests in the meantime |
540e26b
to
cb9c8a0
Compare
/retest |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2827 +/- ##
==========================================
- Coverage 78.83% 78.75% -0.09%
==========================================
Files 1113 1116 +3
Lines 23586 23717 +131
Branches 5940 5972 +32
==========================================
+ Hits 18595 18679 +84
- Misses 4991 5038 +47
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
cb9c8a0
to
55e65d4
Compare
55e65d4
to
57e0c6d
Compare
@andrewballantyne I took this out of WIP, the safety check for the storage status is used, and the feature flag blocks the endpoint |
57e0c6d
to
fac6d6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you would like to talk about permission flows more, let me know
frontend/src/concepts/pipelines/content/artifacts/ArtifactUriLink.tsx
Outdated
Show resolved
Hide resolved
8fd6e8f
to
04a64fd
Compare
frontend/src/concepts/pipelines/content/artifacts/ArtifactUriLink.tsx
Outdated
Show resolved
Hide resolved
09d0651
to
c598110
Compare
c598110
to
740b2d4
Compare
740b2d4
to
cbf9cfe
Compare
/retest |
cbf9cfe
to
be12313
Compare
be12313
to
872a099
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's go with this implementation. We'll be looking at replacing this logic with more structured backend soon, so this is temporary.
We need loading spinners for the artifact peek loading & we definitely need to consider why the Artifact page does not have the peek -- it's awkward that the direct source (the artifact page itself) of the data doesn't have it but the embedded sources (on the runs page) do...
/hold
Not sure if you're addressing the loading spinners now or later, unhold it if you'll follow up in another PR.
chore: Update ArtifactPreview component to show loading spinner during fetch chore: Update ArtifactPreview component to show loading spinner during fetch
872a099
to
c5ecff0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
/unhold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
closes: https://issues.redhat.com/browse/RHOAIENG-7081
Description
How Has This Been Tested?
(reach out to @Gkrumbach07 i you need a test cluster for this PR)
Test Impact
no tests add bc this is a backend route, will add util tests
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main
cc @yannnz