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

Upload a link to the GitHub job #20

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Dec 14, 2023

Currently, the observability server UI links to the workflow run attempt that the upload corresponds to. This is fine in the case where the workflow only contains a single job, as it did in ably-cocoa. But in the case where there’s more than one job in a workflow (e.g. in the case of a matrix build), there’s no way to tell which job an upload corresponds to. So, include this information in the upload.

ably/test-observability#76 adds the observability server functionality for handling these new parameters in the upload.

Resolves #18.

@lawrence-forooghian lawrence-forooghian force-pushed the 18-generate-link-to-matrix-job branch from 53490e8 to db6efa8 Compare December 14, 2023 17:44
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review December 14, 2023 18:07
@lawrence-forooghian lawrence-forooghian marked this pull request as draft December 14, 2023 18:07
lawrence-forooghian added a commit to ably/test-observability that referenced this pull request Dec 14, 2023
Add new github_job_html_url and github_job_api_url parameters to the
upload endpoint, and save them on the upload. Add a link to the
github_job_html_url on the upload details page.

See ably/test-observability-action#20 for an
example of specifying these new parameters.
lawrence-forooghian added a commit to ably/test-observability that referenced this pull request Dec 14, 2023
Add new github_job_html_url and github_job_api_url parameters to the
create upload endpoint, and save them on the upload. Add a link to the
github_job_html_url on the upload details page.

See ably/test-observability-action#20 for an
example of specifying these new parameters.
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review December 14, 2023 18:17
@lawrence-forooghian
Copy link
Collaborator Author

@owenpearson or @VeskeR — I'll also need one of you to update the required checks in this repo’s settings (I don't have the permissions, I'm guessing maybe you do?)

Copy link

@VeskeR VeskeR left a comment

Choose a reason for hiding this comment

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

LGTM! Happy to merge after minor comments are resolved.

I don't have access to set the required checks either. Will probably need @owenpearson for that.

.github/workflows/check.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lawrence-forooghian lawrence-forooghian force-pushed the 18-generate-link-to-matrix-job branch 3 times, most recently from 76d178d to 87a4910 Compare January 10, 2024 14:47
@lawrence-forooghian
Copy link
Collaborator Author

OK, I need to rethink this approach. Turns out that the order in which the jobs are listed in the action YAML file don't necessarily match that in which they are returned from the API — see e.g. https://github.com/ably/test-observability-action/actions/runs/7476602315/job/20347351917?pr=20.

@VeskeR
Copy link

VeskeR commented Jan 10, 2024

OK, I need to rethink this approach. Turns out that the order in which the jobs are listed in the action YAML file don't necessarily match that in which they are returned from the API — see e.g. https://github.com/ably/test-observability-action/actions/runs/7476602315/job/20347351917?pr=20.

Wow, that's unexpected. Yeah, the API request for the workflow mentioned has first-job as second element in the array.

I couldn't find any other way to get a unique identifier for a job either.

It seems that the next best thing is to find the correct job in the API response by its name. There is a github.job prop for that, but it does not include matrix information, whereas in API response name prop is shown as [job name] ([matrix.prop1], [matrix.prop2], ...).
Technically, we can concatenate the correct job name with matrix values (unless order of matrix properties is also not guaranteed) and expose it via JOB_NAME (instead of JOB_INDEX) and pass it to the action. It's not that far fetched from index calculation that we currently do with offsets and stuff.

@lawrence-forooghian WDYT?

@lawrence-forooghian lawrence-forooghian force-pushed the 18-generate-link-to-matrix-job branch 3 times, most recently from c246daf to 3a486ca Compare January 22, 2024 17:45
@lawrence-forooghian
Copy link
Collaborator Author

@VeskeR I've implemented your suggestion of using the job’s name. Would you be able to take another look, please?

Currently, the observability server UI links to the workflow run attempt
that the upload corresponds to. This is fine in the case where the
workflow only contains a single job, as it did in ably-cocoa. But in the
case where there’s more than one job in a workflow (e.g. in the case of
a matrix build), there’s no way to tell which job an upload corresponds
to. So, include this information in the upload.

I originally tried implementing this by passing a job-index input, whose
value was the index to which the current job corresponds in the response
from the "list jobs for a workflow run attempt" GitHub API. However,
some experimentation then showed that that the order in which the jobs
are listed in the action YAML file doesn’t necessarily match that in
which they are returned from the API, so there was no way to calculate
the value to pass for job-index. Hence, switched to using the job’s
`name` instead.

Resolves #18.
@lawrence-forooghian lawrence-forooghian force-pushed the 18-generate-link-to-matrix-job branch from 3a486ca to 518bc5e Compare January 23, 2024 09:37
Copy link

@VeskeR VeskeR left a comment

Choose a reason for hiding this comment

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

LGTM

@lawrence-forooghian lawrence-forooghian merged commit 86d42e5 into main Jan 24, 2024
3 checks passed
@lawrence-forooghian lawrence-forooghian deleted the 18-generate-link-to-matrix-job branch January 24, 2024 14:04
@VeskeR VeskeR restored the 18-generate-link-to-matrix-job branch February 2, 2024 00:16
@VeskeR VeskeR deleted the 18-generate-link-to-matrix-job branch February 2, 2024 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Generate link to matrix job
2 participants