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

Add artifact access via graph proxy #288

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yousefmoazzam
Copy link
Collaborator

No description provided.

@garryod garryod added enhancement New feature or request rust Pull request that updates Rust code labels Nov 22, 2024
@yousefmoazzam yousefmoazzam force-pushed the graph-proxy-artifact-access branch 3 times, most recently from a579aac to 2357b86 Compare November 28, 2024 15:57
@yousefmoazzam
Copy link
Collaborator Author

I'm not overly pleased about passing the argo workflows server URL from WorkflowsQuery::workflows all the way down to Task::new, it feels clumsy, but I didn't see a way to get the server URL to the Task constructor otherwise.

If there's a better way to do that then I'm happy to change this. I have a sneaking suspicion that the rejigging of the design as mentioned in the thread above may be better done sooner rather than later though, to avoid having to deal with this.

@yousefmoazzam
Copy link
Collaborator Author

If it helps checking that this works, I've been using the following query to test things:

{
  workflows(visit: {proposalCode: "mg", proposalNumber: 36964, number: 1}){
    nodes {
      name
      status {
        ...on WorkflowSucceededStatus {
          tasks {
            name,
            artifacts {
              name
              url
            }
          }
        }
      }
    }
  }
}

@yousefmoazzam yousefmoazzam marked this pull request as ready for review November 28, 2024 16:10
graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
@garryod
Copy link
Member

garryod commented Nov 29, 2024

I'm not overly pleased about passing the argo workflows server URL from WorkflowsQuery::workflows all the way down to Task::new, it feels clumsy, but I didn't see a way to get the server URL to the Task constructor otherwise.

If there's a better way to do that then I'm happy to change this. I have a sneaking suspicion that the rejigging of the design as mentioned in the thread above may be better done sooner rather than later though, to avoid having to deal with this.

Another option would be to have the url field use a resolver, and store the additional information needed to construct the path in the Artifact

@yousefmoazzam yousefmoazzam marked this pull request as draft December 9, 2024 10:37
@yousefmoazzam yousefmoazzam force-pushed the graph-proxy-artifact-access branch from 2357b86 to ef580c8 Compare January 27, 2025 16:07
@yousefmoazzam yousefmoazzam changed the base branch from main to refactor-workflow-resolvers January 27, 2025 16:07
@yousefmoazzam yousefmoazzam marked this pull request as ready for review January 27, 2025 16:20
@yousefmoazzam yousefmoazzam force-pushed the refactor-workflow-resolvers branch 3 times, most recently from 285168e to 4ae0ebb Compare January 29, 2025 16:11
@yousefmoazzam yousefmoazzam marked this pull request as draft January 29, 2025 16:20
@yousefmoazzam yousefmoazzam force-pushed the refactor-workflow-resolvers branch from 666f2ae to d484744 Compare January 30, 2025 13:29
@yousefmoazzam yousefmoazzam force-pushed the graph-proxy-artifact-access branch from ef580c8 to f1d8f58 Compare January 31, 2025 09:51
@yousefmoazzam yousefmoazzam marked this pull request as ready for review January 31, 2025 10:01
@yousefmoazzam yousefmoazzam marked this pull request as draft January 31, 2025 11:55
Base automatically changed from refactor-workflow-resolvers to main January 31, 2025 12:15
@yousefmoazzam yousefmoazzam force-pushed the graph-proxy-artifact-access branch from f1d8f58 to 1977352 Compare January 31, 2025 13:23
@yousefmoazzam
Copy link
Collaborator Author

yousefmoazzam commented Jan 31, 2025

While I'm fairly sure the filename is what we want to give requestors rather than the artifact name which argo workflows uses, maybe we'd want to provide the artifact name, so I left the two commits separate for easier viewing of using one vs. the other. Can squash if the filename is what we want.

Also, lots of assumptions being made in getting the filename:

  1. argo workflow artifacts always stored in an s3 bucket (probably true, config seems to indicate this, but maybe a better way to handle this)
  2. the "key" of an artifact should be non-null (seems reasonable, but I don't know much about s3 buckets, I went off of the fact that a "key" looks like a filepath relative to a bucket)
  3. an artifact "key" shouldn't have .. at the end of it
  4. filename should be valid utf-8

@yousefmoazzam yousefmoazzam marked this pull request as ready for review January 31, 2025 13:30
Copy link
Member

@garryod garryod left a comment

Choose a reason for hiding this comment

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

Looking good, could make more use of error types though, just don't want to crash in prod

graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
@yousefmoazzam yousefmoazzam force-pushed the graph-proxy-artifact-access branch from 1977352 to a52c7ea Compare January 31, 2025 15:46
garryod
garryod previously approved these changes Jan 31, 2025
Copy link
Member

@garryod garryod left a comment

Choose a reason for hiding this comment

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

LGTM

graph-proxy/src/graphql/workflows.rs Outdated Show resolved Hide resolved
@yousefmoazzam yousefmoazzam force-pushed the graph-proxy-artifact-access branch 2 times, most recently from e8dadeb to dbbaebe Compare January 31, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rust Pull request that updates Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants