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.

Sign up for GitHub

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

Jump to bottom

feature: Add integrated issue handling #176

Merged
merged 6 commits into from Jan 29, 2024
Merged

feature: Add integrated issue handling #176

merged 6 commits into from Jan 29, 2024

Conversation

jku
Copy link
Member

@jku jku commented Jan 24, 2024 •
edited

Open and close issues based on workflow successes:

  • If a TUF-on-CI workflow fails and issue does not exist, file an issue
  • If issue for the workflow exists, add a comment
  • If a TUF-on-CI workflow succeeds and issue exists for workflow, close issue

Some notes:

Fixes #168

@jku
Copy link
Member Author

jku commented Jan 24, 2024 •
edited

TODO: Expand the message (these are likely different for different workflows):

  • what will be done automatically to resolve
  • what can be done by who to manually resolve
  • what is the timeline to repository failure (like "Unless the online-signing issue is resolved, breakage will be visible to clients in 7 days (Sun 28 Jan 2024 16:22 UTC)")

@jku jku force-pushed the issue-mgmt branch 3 times, most recently from bdea249 to 4a5e561 Compare January 24, 2024 17:14
@jku
Copy link
Member Author

jku commented Jan 24, 2024

I think this is ready for first review?

  • I'd love to have timeline to repository failure but it's tricky -- would have to know the current fully published timestamp expiry. I'm dropping this idea from this first version
  • The javascript is a little more complicated than I wish but manageable
  • There are some possible GitHub Actions crimes in this PR (see Make a copy of update-issue action step in all of the actions)

Example issue jku/tuf-on-ci-sigstore-test#19

@jku jku marked this pull request as ready for review January 24, 2024 17:20
@jku
Copy link
Member Author

jku commented Jan 25, 2024 •
edited

Some reflection: The idea of "hiding" the error handling within the actions does keep workflows tidy but has some flaws. This example is the best:

  • What if actions/deploy-pages fails in the publish workflow? We should react to that...
  • Users will want to add their own "final deploy job". We should also alert if that fails

I have to rethink and maybe undo the crimes against GitHub Actions in this PR.

@jku jku marked this pull request as draft January 25, 2024 12:28
@jku
Copy link
Member Author

jku commented Jan 25, 2024

I'll modify this so that

  • workflows are expected to call update-issue as the last job in the workflow
  • the tricks in individual actions currently in the PR are removed

@jku
actions: Add integrated issue management
0bc0b00 
Goals:
* If workflow fails and issue does not exist, file an issue
* If issue for the workflow exists, add a comment
* If workflow succeeds and issue exists for workflow, close issue

This adds a new action update-issue which we expect all workflows
(except possibly signing-event) to call as the last job.
@jku
actions: Use "--quiet" with pip
696198c 
The pip install log is massively long in the logs: make it a little more
readable.
@jku
actions: Improve issue comment in update-issue
5563954
@jku jku mentioned this pull request Jan 25, 2024
Merged
@jku

This comment was marked as outdated.

Sign in to view
@jku
actions: Always run test on publish branch
c9d91e5 
This is the only thing that makes sense:
* publish branch documents what should be published
* test action compares the published metadata to the
  "expected metadata" in sources on this branch

This fixes the potential issue where cron workflows always run
on main and could then use the wrong content as "expected
metadata".
@jku jku marked this pull request as ready for review January 26, 2024 08:18
@jku
Copy link
Member Author

jku commented Jan 26, 2024

Would appreciate review comments on this and the template PR (see description for links).

@kommendorkapten
Copy link
Member

I think having an explicit job that reports error is good. It's explicit and IMHO does not lead to any issues.

@kommendorkapten
Copy link
Member

Following up the discussion we had in slack on adding some custom message into the issue comment, any ideas on that?

@jku
Copy link
Member Author

jku commented Jan 26, 2024 •
edited

Following up the discussion we had in slack on adding some custom message into the issue comment, any ideas on that?

  • idea is good IMO
  • I think whatever the implementation of that is, it can be a separate PR and does not need to be included here. The two options I can see are:
    • custom content is defined in a repository variable like TUF_ON_CI_ISSUE_INCLUDE. This is nice in that the git repository stays as "just the content": metadata and artifacts
    • we define a directory in the repo (like .tuf-on-ci) where we can put repository configuration like this and then use read the custom content from .tuf-on-ci/issue-include.md. I'm suggesting a dot-directory because I think signers do not need to see repository configuration like this in the git repo

@jku
Copy link
Member Author

jku commented Jan 26, 2024

I think having an explicit job that reports error is good. It's explicit and IMHO does not lead to any issues.

It looks OK but I think the downsides are real:

  • the workflow will require more changes (e.g. if you add a test-with-sistore-client job in the test workflow you will then need to modify the update-issue jobs needs property) and there are more things that can be changed
  • We know from experience that tuf-on-ci occasionally requires workflow changes

Merging the customizations and the required changes from tuf-on-ci upgrades is now on the repository maintainer... This is not optimal. Still, I think this is the best option we have.

jku
jku commented Jan 27, 2024
View reviewed changes
actions/update-issue/action.yml Outdated Show resolved Hide resolved
@jku
actions: Fix bug with labeling
43912c7 
context.payload does not contain 'workflow' for all event types.
Use the actual workflow name instead: it looks like GitHub labels
allow just about anything.

Remove the custom message for specific workflows: this was on the verge
of too complicated already, now it's over the line.

Fix a typo in issue comment content.
@jku
Lint fixes
7136859 
* These are unrelated to the PR, they are from black 24.1
* We should start pinning test deps but I'm not doing it here
@kommendorkapten
Copy link
Member

Merging the customizations and the required changes from tuf-on-ci upgrades is now on the repository maintainer... This is not optimal. Still, I think this is the best option we have.

Agreed.

We already have some customizations to our workflows, so I'm already used to patch them with updates from tuf-on-ci-template

This was referenced Jan 29, 2024
Merged
Closed
@jku jku linked an issue Jan 29, 2024 that may be closed by this pull request
Make workflow changes for repository smoke test #171
Closed
@jku
Copy link
Member Author

jku commented Jan 29, 2024

Thanks for review. I will merge this and file an issue to track the custom issue comment content

@jku jku merged commit 9eb98c9 into theupdateframework:main Jan 29, 2024
1 check passed
@jku jku mentioned this pull request Jan 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kommendorkapten kommendorkapten kommendorkapten approved these changes

@lukpueh lukpueh Awaiting requested review from lukpueh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make workflow changes for repository smoke test File issues when workflows/actions fail
2 participants
@jku @kommendorkapten