fix: Add missing 'archived' prop for ArtifactPanel component. Fixes #12331 #12397
Conversation
|
Screenshot example? |
I'll provide it in a moment. |
@jmeridth I added the screenshots. |
…xes an issue where the UI returns an Internal Server Error. Fixes argoproj#12331 Signed-off-by: Garett MacGowan <[email protected]>
Signed-off-by: Garett MacGowan <[email protected]>
Garett-MacGowan
force-pushed
the
fix/archived_workflow_artifact
branch
from
c8799f2
to
f4405b0
Compare
Signed-off-by: Garett MacGowan <[email protected]>
Signed-off-by: Garett MacGowan <[email protected]>
Signed-off-by: Garett MacGowan <[email protected]>
|
@agilgur5 this is rebased and ready to go |
test/e2e/argo_server_test.go
Outdated
| @@ -1037,6 +1037,30 @@ spec: | |||
| }) | |||
| } | |||
|
|
|||
| func (s *ArgoServerSuite) TestArtifactServerArtifactPassing() { | |||
There was a problem hiding this comment.
I feel like we should specify that this checking Archived Workflows specifically, though I do see that some of the tests name it explicitly and others don't 😕
There was a problem hiding this comment.
@agilgur5 I changed the name to TestArtifactServerArchivedWorkflow. Is this more along the lines of what you were thinking?
There was a problem hiding this comment.
Works for me. Naming is hard, especially for this level of specificity 😅
|
Also is there a reason you merge |
|
Btw @Garett-MacGowan would you be interested in joining the Argo project as a Member? |
@agilgur5 I'd be happy to! There's no strict hourly commitment right? I'd be happy to spend a few hours every week. |
Didn't know PRs didn't need to be up to date. If there are conflicts, it would provide a different prompt than "Update branch" right? |
…chivedWorkflow for clarity. Addresses PR comment. Fixes argoproj#12331 Signed-off-by: Garett MacGowan <[email protected]>
It is a requirement you can add on GitHub, but we don't currently require it. There's only been a handful of times where that's caused an issue, which is actually with "invisible" merge conflicts (i.e. two PRs that when merged together break a build, but do not conflict in version control itself). I think we can also solve those with a merge queue as well instead of an extra PR requirement, as was discussed briefly over Slack
Yep, it would say merging is not possible due to conflicts. The prompt instead says "Resolve conflicts" in that case. |
Awesome! Correct, there are currently no strict commitments associated with Membership or other roles. It comes with some additional privileges (i.e. issue triage) and some small responsibilities that are usually the norm for active contributors in most repos anyway (e.g. responding when tagged). Otherwise, the only main "rule" outlined in the doc is that Members with no activity what-so-ever for an entire year will get pruned (but can still re-request whenever and will be expedited if so). Could you file a membership request issue in the We do also have a separate Sustainability Effort that was recently created that is specific to Argo Workflows (i.e. does not apply to CD or other projects at this time). That effort is a completely optional addition to the formal Argo project roles. If you're interested in that as well, that does have an hourly commitment but is not "strict" per se, as it is an average over time (e.g. I had 0 hours some weeks in Nov/Dec but my average is much higher). |
|
For reference, I'm not an Approver (yet), so I don't have merge permissions, which is why I requested a review from Julie. Agreed this would have been good to have for the release, but we'll have another soon enough since there are still two P1 bugs out. |
Sorry, I missed that you requested my review! |
|
Thanks Julie! |
…12331 (#12397) Signed-off-by: Garett MacGowan <[email protected]> Signed-off-by: Saravanan Balasubramanian <[email protected]>
…rgoproj#12331 (argoproj#12397) Signed-off-by: Garett MacGowan <[email protected]>
…rgoproj#12331 (argoproj#12397) Signed-off-by: Garett MacGowan <[email protected]>
…rgoproj#12331 (argoproj#12397) Signed-off-by: Garett MacGowan <[email protected]>
…rgoproj#12331 (argoproj#12397) Signed-off-by: Garett MacGowan <[email protected]> Signed-off-by: Isitha Subasinghe <[email protected]>
Fixes #12331
Motivation
A missing
archivedprop in the UI'sArtifactPanelcomponent was causing an improper URL to be used to fetch the artifact (assuming the workflow was archived).Modifications
Added the missing
archivedprop to theArtifactPanelcomponent.Verification
I added tests to the argo server test suite to account for the URL structure which was previously untested. I called this test
GetArtifactByNodeID, in accordance with naming conventions in similar tests.For the UI, I ran the
TestArtifactServerArtifactPassingtest (which contains theGetArtifactByNodeIDtest mentioned above) using:Once it completed, I manually verified that the artifact in the ArtifactPanel was downloadable.
(download button)
