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 workflow_dispatch github event handling and upgrade rpc #81

Closed

Conversation

devspyrosv
Copy link
Contributor

@devspyrosv devspyrosv commented Jul 7, 2023

Requirements:

  • A self-hosted runner should handle the workflow_dispatch event.
  • The repo-policy-compliance (rpc for short) should be updated to the latest version on charm upgrade.

This PR modifies pre-job.j2 allowing it to differentiate between different github events, handles workflow_dispatch and logs useful variables to syslog.
Also the repo-policy-compliance package is upgraded on upgrade_charm.

@devspyrosv devspyrosv requested a review from a team as a code owner July 7, 2023 20:11
@devspyrosv devspyrosv changed the title Add workflow_dispatch github event handling Add workflow_dispatch github event handling and upgrade rpc Jul 7, 2023
@devspyrosv
Copy link
Contributor Author

Maybe this is relevant too #82

src/charm.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jdkandersson jdkandersson left a comment

Choose a reason for hiding this comment

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

Nice work!

src/charm.py Outdated Show resolved Hide resolved
templates/pre-job.j2 Show resolved Hide resolved
templates/pre-job.j2 Outdated Show resolved Hide resolved
@jdkandersson
Copy link
Contributor

I didn't see the triggering of the workflow dispatch during the e2e tests?

@devspyrosv
Copy link
Contributor Author

I didn't see the triggering of the workflow dispatch during the e2e tests?

Yes it's not triggered as I was not sure if it should be added there. On it!

src/charm.py Outdated Show resolved Hide resolved
templates/pre-job.j2 Outdated Show resolved Hide resolved
templates/pre-job.j2 Outdated Show resolved Hide resolved
templates/pre-job.j2 Outdated Show resolved Hide resolved
Copy link

@gregory-schiano gregory-schiano left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll put a comment here just as a note, please modify the e2e tests to trigger workflow dispatch and check that it worked

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I just talked with David, would you like to merge the current workflow_dispatch feature as it has been tested manually already. And I will revive an old discarded e2e test that include the workflow_dispatch test in another pull request. In that case we don't need to repeat implementing the same test twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will clean up the commits first. I will remove those I used to attempt to make the tests work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weiiwang01 done!

@devspyrosv
Copy link
Contributor Author

devspyrosv commented Jul 10, 2023

Allowing the workflow with content:

name: Workflow Dispatch Tests

on:
  workflow_dispatch: {}

jobs:
  workflow-dispatch-tests:
    runs-on: [ self-hosted, linux, x64]
    steps:
      - name: Run a one-line script
        run: echo Hello, world of workflow dispatches!

(sha256sum: 1e27bd8a2ebddedbc0d609c8bd804f6a2076e577ca8499ec8f8ac79aa340af77)

to be run.

/canonical/self-hosted-runners/run-workflows 99a30536ed9311cfa0b7b89af94326d97f91b5cd

templates/pre-job.j2 Outdated Show resolved Hide resolved
amandahla
amandahla previously approved these changes Jul 10, 2023
Copy link
Contributor

@amandahla amandahla left a comment

Choose a reason for hiding this comment

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

LGTM :-)

@github-actions
Copy link
Contributor

Test coverage for 6232241

Name                                    Stmts   Miss Branch BrPart  Cover   Missing
-----------------------------------------------------------------------------------
src/charm.py                              371     86     86     21    74%   126-128, 182-190, 202-206, 275->279, 300-305, 315, 324-325, 334-345, 362-364, 372-375, 381, 392, 403->420, 408-413, 430-431, 435-436, 441-444, 457-458, 460-461, 495-503, 514-515, 533-535, 538->exit, 542-544, 567-572, 590-591, 593-594, 596-597, 629->631, 690-691, 704, 717
src/errors.py                              22      0      0      0   100%
src/event_timer.py                         42      8      4      0    74%   96-99, 119-122
src/firewall.py                            43     25     10      0    38%   38-42, 64-67, 75-149
src/github_type.py                         36      0      0      0   100%
src/lxd_type.py                            38      0      2      0   100%
src/repo_policy_compliance_service.py       5      5      0      0     0%   4-14
src/runner.py                             245     29     74     21    84%   40->44, 184-191, 197-203, 262-267, 272, 292, 325->328, 331-333, 340, 354, 364, 368, 370, 385, 429-434, 444, 528, 561, 587, 592-604, 618, 636
src/runner_manager.py                     203     24     80      8    87%   196-198, 211-212, 224-226, 232-237, 241-242, 252-253, 306, 331-335, 354, 372, 488, 508
src/runner_type.py                         48      0     10      0   100%
src/utilities.py                           64      4     16      6    88%   74->76, 78->84, 89-91, 157, 210
-----------------------------------------------------------------------------------
TOTAL                                    1117    181    282     56    81%

Static code analysis report

Run started:2023-07-11 10:14:16.677532

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 2496
  Total lines skipped (#nosec): 0
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 9

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@devspyrosv
Copy link
Contributor Author

This PR was "moved" to #84 as it could not use the secrets when running the workflows.

The reason was that this was an external (read: from my own account) one.

Closing

@devspyrosv devspyrosv closed this Jul 11, 2023
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.

5 participants