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

Improvements in signadot job get #130

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

Conversation

daniel-de-vera
Copy link
Contributor

This improves the output of signadot job get.
It fixes https://github.com/signadot/signadot/issues/4522 and includes the artifacts as part of the yaml and json output.

E.g.:

$ ./signadot job get basic-eyyqvxy
Job Name:           basic-eyyqvxy
Job Runner Group:   basic
Status:             Failed
Exit Code:          0
Message:            could not upload artifact /tmp/job902155206/stdout.index, 401 Unauthorized: unauthorized
Environment:        baseline
Created At:         2 hours ago
Started At:         2 hours ago
Duration:           4m15s
Dashboard URL:      https://app.signadot.com/testing/jobs/basic-eyyqvxy

Artifacts
No artifacts
$ ./signadot job get basic-tvkbzqq
Job Name:           basic-tvkbzqq
Job Runner Group:   basic
Status:             Succeeded
Environment:        baseline
Created At:         2 hours ago
Started At:         2 hours ago
Duration:           50s
Dashboard URL:      https://app.signadot.com/testing/jobs/basic-tvkbzqq

Artifacts
PATH      SIZE
aaa.txt   20 B
@stderr   0 B
@stdout   1.3 kB
$ ./signadot job get basic-tvkbzqq -o yaml
spec:
  command: null
  env: null
  namePrefix: basic
  runnerGroup: basic
  script: |
    #!/bin/bash

    echo "Hello this is the demo meeting"
    start=`date +%s`

    x=1
    while [ $x -le 45 ]
    do
      echo "Welcome $x times (env TEST=$TEST)"
      x=$(( $x + 1 ))
      sleep 1
    done
    echo "This is an artifact" > aaa.txt
    echo "We are done!"

    end=`date +%s`
    runtime=$((end-start))
    echo $runtime
  uploadArtifact:
  - meta:
      format: text
    path: aaa.txt
status:
  artifacts:
  - path: aaa.txt
    size: 20
  - path: '@stderr'
    size: 0
  - path: '@stdout'
    size: 1343
  createdAt: "2024-06-13T13:53:42Z"
  executionCount: 1
  finishedAt: "2024-06-13T13:54:32Z"
  phase: succeeded
  startedAt: "2024-06-13T13:53:44Z"
  state:
    succeeded: {}

@scott-cotton
Copy link
Member

Some notes that I think should be discussed:

The Job resources yaml is in this PR not forward compatible with having multiple attempts. I thought we wanted to leave that option open without compatibility issues at the level of parsing it.

The Job resource yaml in this PR is also not compatible with the go-sdk (nor other sdk) Jobs, which I guess makes it harder to use.

Can we remove tries from go-sdk?

@daniel-de-vera
Copy link
Contributor Author

Can we remove tries from go-sdk?

I'm fine with that. But I would keep artifacts as part of the display status.
I find it odd, that we are displaying artifacts in one view (default view), but not in the others.

@scott-cotton
Copy link
Member

Can we remove tries from go-sdk?

I would keep artifacts as part of the display status. I find it odd, that we are displaying artifacts in one view (default view), but not in the others.

I agree, but I think that needs to be balanced with the non-uniformities it introduces: It is also odd that artifacts are not part of the apiserver status response nor the [go-]sdk models but are output from the cli.

It kind-of breaks the whole document model to piece together resources from various endpoints: the notion of the resource struct becomes fragmented and harder to manage, use, and reason about. Practically, a Job becomes no longer a Job but a cli Job vs an SDK job.

The restructuring of attempts from array to embedding directly in Job contributes to that as well as the artifacts.

That being said, I also agree and do think it's odd that the cli displays artifacts in some views but not others.

I think that it is worth discussing the relationship between solutions before committing to a format that may well entail compatibility baggage.

@jmsktm
Copy link
Contributor

jmsktm commented Jun 13, 2024

I think we should definitely not print artifacts information fetched separately on signadot job get <job-id> -o yaml. That seems like a violation printing more than what's expected in a regular GET /job/{id} call. We could rather not display artifacts in the regular view if it's odd. IIRC Anirudh wanted to keep the display of artifacts separate as well.

@jmsktm
Copy link
Contributor

jmsktm commented Jun 13, 2024

Having said that, displaying that on explicitly passing a --show-artifacts switch in the regular (formatted) view seems reasonable. But still not for the -o yaml|json.

@daniel-de-vera daniel-de-vera mentioned this pull request Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants