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 authorize step to both PR workflows #2644

Closed
wants to merge 6 commits into from

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Oct 20, 2022

More information: iterative/cml#574 (comment)

image

Internal

image

External

image

@shcheklein
Copy link
Member

@0x2b3bfa0 hey, as an expert in this, could you please review and see that we are not missing anything and/or there are better ways to organize it now (run tests for external contributions after an approval).

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Oct 20, 2022

@mattseddon wrote me a Slack direct message a dozen hours ago asking the same, hence e4e23aa 👍🏼

See also

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🎉

@mattseddon
Copy link
Member Author

@0x2b3bfa0 shouldn't the checks be run when I open a PR?

@mattseddon
Copy link
Member Author

@0x2b3bfa0 shouldn't the checks be run when I open a PR?

Because the checks are run in the context of the base branch authorize is not available. See #2645 for when the checks do get run. Will need to merge this in two parts. With on: pull_request and then remove that trigger afterwards.

@mattseddon
Copy link
Member Author

mattseddon commented Oct 20, 2022

Code Climate has analyzed commit 9c59ba3 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.8% (0.0% change).

View more on Code Climate.

This seems like a bug in paambaati/[email protected]

The problem is here:

https://github.com/paambaati/codeclimate-action/blob/8788c820257f80343c47eb820a5f1cd9763e3faf/src/main.ts#L38

The line needs to be extended to include pull_request_target

@mattseddon
Copy link
Member Author

Raised paambaati/codeclimate-action#627. This is blocked until that gets resolved.

@mattseddon mattseddon added the blocked Issue or pull request blocked due to other dependencies or issues label Oct 20, 2022
@0x2b3bfa0
Copy link
Member

It's hacky, but you could use an env block to override those environment variables for the action.

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Oct 21, 2022

E.g.

      - uses: paambaati/[email protected]
        env:
          CC_TEST_REPORTER_ID: ${{secrets.CC_TEST_REPORTER_ID}}
          GITHUB_EVENT_NAME: ${{ github.event_name == 'pull_request_target' && 'pull_request' || github.event_name }}
        with:
          coverageCommand: xvfb-run -a yarn run cover
          coverageLocations: ${{github.workspace}}/coverage/lcov.info:lcov

@mattseddon mattseddon force-pushed the add-authorize-to-pr-workflows branch 2 times, most recently from 31c513a to 0af92c1 Compare October 21, 2022 02:34
@@ -0,0 +1,834 @@
export const data = {
Copy link

Choose a reason for hiding this comment

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

File fun.ts has 834 lines of code (exceeds 300 allowed). Consider refactoring.

],
author_association: 'COLLABORATOR',
auto_merge: null,
base: {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

deletions: 2,
diff_url: 'https://github.com/iterative/vscode-dvc/pull/2645.diff',
draft: true,
head: {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@mattseddon mattseddon force-pushed the add-authorize-to-pr-workflows branch 3 times, most recently from 9a2d16c to e900467 Compare October 21, 2022 03:49
@mattseddon
Copy link
Member Author

E.g.

      - uses: paambaati/[email protected]
        env:
          CC_TEST_REPORTER_ID: ${{secrets.CC_TEST_REPORTER_ID}}
          GITHUB_EVENT_NAME: ${{ github.event_name == 'pull_request_target' && 'pull_request' || github.event_name }}
        with:
          coverageCommand: xvfb-run -a yarn run cover
          coverageLocations: ${{github.workspace}}/coverage/lcov.info:lcov

It doesn't seem like overriding the environment variables like this has any effect on the action 😞.

@codeclimate
Copy link

codeclimate bot commented Oct 21, 2022

Code Climate has analyzed commit a217a20 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.8% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon force-pushed the add-authorize-to-pr-workflows branch from ac9df7a to 433b9a5 Compare November 20, 2022 22:03
@mattseddon mattseddon closed this Nov 23, 2022
@mattseddon mattseddon deleted the add-authorize-to-pr-workflows branch November 26, 2022 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Issue or pull request blocked due to other dependencies or issues 🏠 housekeeping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants