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

feat: GitHub Action customizations #382

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

dnys1
Copy link

@dnys1 dnys1 commented May 13, 2022

Adds support for an action block which can be attached to tasks to specify GitHub Action context, e.g. uses, with, etc. Instead of a separate block, this config could be in-lined but I wasn't sure if that was in keeping with the library, since providers other than GitHub are supported.

stages:
  - custom_step:
      - command:
          - ./script_a
          - ./script_b
          - ./script_c
        action:
          id: custom-scripts
          uses: ./.github/actions/my-action
          with:
            my-key: my-var
          if: always()
          working-directory: ./tool
          shell: fish

These are just some thoughts - looking forward to your feedback!

@kevmoo
Copy link
Collaborator

kevmoo commented May 13, 2022

thoughts @jakemac53 @natebosch ?

'$package; ${task.command}',
_commandForOs(task.command),
id: task.action?.id,
ifCondition: task.action?.condition ??
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should likely add the && steps.$pubStepId.conclusion == 'success' no matter what?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good to me. I agree that in most cases, this is the desired behavior, and for the few "failure-only" scripts you may want to run, they can be handled via the on_completion task.

workingDirectory: package,
'$package; ${task.command}',
_commandForOs(task.command),
id: task.action?.id,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a bit concerned with hard coding all the supported fields we allow overriding here.

Would it be reasonable to just treat this as a more opaque override map? I don't think we want to be fielding feature requests/prs for every thing people end up wanting to override here.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good question. At that point, does it make sense for action to function more similar to command and spread its JSON into the GitHub action yaml so that, for example, overriding run becomes possible? Right now, the following is unclear in my mind:

stages:
  - custom_step:
      - command: ./script_a
        action:
            run: ./script_b

But this could possibly make more sense:

stages:
  - custom_step:
      - action:
           run: ./script_b

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, possibly just allowing a fully custom action task would make the most sense? I kind of like that.

Copy link
Author

Choose a reason for hiding this comment

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

Sweet! I’ll work on updating it 👍

Copy link
Author

Choose a reason for hiding this comment

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

Okay, so I've made the following changes:

  • action now functions similar to command as a top level step type
  • Consolidated GitHub action config into the GitHubActionConfig class since there was a lot of overlap with _CommandEntry
  • Added some validators for known GitHub step config keys (run, uses, with, etc.) but allows for unknown keys to be used as well. Let me know if these helpers are useful or if we should just dump whatever we're given. There is the possibility for things to change, and the flipside is that I think it offers a bit nicer DX than discovering a typo down the road.

@dnys1
Copy link
Author

dnys1 commented May 21, 2022

Hey @jakemac53 - still feeling like this is a good direction? I've updated the action specifier to behave like command as noted above.

@jakemac53
Copy link
Collaborator

cc @natebosch or @kevmoo opinions here?

@kevmoo
Copy link
Collaborator

kevmoo commented Jun 23, 2022

We want to land this? @jakemac53 ?

@kevmoo
Copy link
Collaborator

kevmoo commented Jul 11, 2022

@dnys1 – you'll need to rebase this work on latest. Sorry!

@dnys1 dnys1 force-pushed the feat/actions-config branch from 2fc2ce6 to f200385 Compare July 16, 2022 19:22
@dnys1
Copy link
Author

dnys1 commented Jul 16, 2022

Hey @kevmoo, no worries and thanks for the heads up. I've rebased off the latest changes.

@kevmoo kevmoo force-pushed the feat/actions-config branch 2 times, most recently from dc5436d to bc155cb Compare July 18, 2022 19:47
@coveralls
Copy link

coveralls commented Jul 18, 2022

Pull Request Test Coverage Report for Build 2693092847

  • 102 of 116 (87.93%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 83.912%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mono_repo/lib/src/commands/github/step.g.dart 11 12 91.67%
mono_repo/lib/src/package_config.dart 5 6 83.33%
mono_repo/lib/src/github_config.dart 38 50 76.0%
Totals Coverage Status
Change from base Build 2679237497: -0.2%
Covered Lines: 1304
Relevant Lines: 1554

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2693043484

  • 101 of 115 (87.83%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 83.912%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mono_repo/lib/src/commands/github/step.g.dart 11 12 91.67%
mono_repo/lib/src/package_config.dart 5 6 83.33%
mono_repo/lib/src/github_config.dart 38 50 76.0%
Files with Coverage Reduction New Missed Lines %
mono_repo/lib/src/task_type.dart 2 93.94%
Totals Coverage Status
Change from base Build 2679237497: -0.2%
Covered Lines: 1304
Relevant Lines: 1554

💛 - Coveralls

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2022

Codecov Report

Merging #382 (bc155cb) into master (53aa5b2) will decrease coverage by 76.96%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #382       +/-   ##
==========================================
- Coverage   84.10%   7.13%   -76.97%     
==========================================
  Files          34      34               
  Lines        1453    1555      +102     
==========================================
- Hits         1222     111     -1111     
- Misses        231    1444     +1213     
Impacted Files Coverage Δ
mono_repo/lib/src/commands/github/github_yaml.dart 0.00% <0.00%> (-96.70%) ⬇️
mono_repo/lib/src/commands/github/step.dart 0.00% <0.00%> (-100.00%) ⬇️
mono_repo/lib/src/commands/github/step.g.dart 0.00% <0.00%> (-100.00%) ⬇️
mono_repo/lib/src/github_config.dart 0.00% <0.00%> (-98.37%) ⬇️
mono_repo/lib/src/package_config.dart 0.00% <0.00%> (-95.63%) ⬇️
mono_repo/lib/src/task_type.dart 10.60% <0.00%> (-82.62%) ⬇️
mono_repo/lib/src/raw_config.g.dart 0.00% <0.00%> (-100.00%) ⬇️
mono_repo/lib/src/mono_config.g.dart 0.00% <0.00%> (-100.00%) ⬇️
mono_repo/lib/src/ci_test_script.dart 0.00% <0.00%> (-100.00%) ⬇️
mono_repo/lib/src/package_config.g.dart 0.00% <0.00%> (-100.00%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53aa5b2...bc155cb. Read the comment docs.

@kevmoo kevmoo force-pushed the feat/actions-config branch from bc155cb to db9505a Compare July 18, 2022 19:52
Two issues fixed:
1. `name` overrides were not being passed correctly to methods/ctors - whoops!
2. Default `name` should consider either `run` or `uses` depending on which is passed.
@provokateurin
Copy link

Hi, any plans to continue this? I would love to have this feature implemented.

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.

6 participants