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

ci(pr-test): use GitHub API to get changed files + diff #21305

Merged
merged 2 commits into from
Dec 8, 2022
Merged

ci(pr-test): use GitHub API to get changed files + diff #21305

merged 2 commits into from
Dec 8, 2022

Conversation

yin1999
Copy link
Member

@yin1999 yin1999 commented Oct 5, 2022

Description

Reflect of mdn/translated-content#8952. Try to speed up the PR test workflow.

Additional details

Please see the original PR: mdn/translated-content#8952.

@yin1999 yin1999 requested a review from a team as a code owner October 5, 2022 07:00
@yin1999 yin1999 closed this Oct 5, 2022
@yin1999 yin1999 reopened this Oct 5, 2022
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Let's wait until mdn/translated-content#8952 has landed and works without issues.

@caugner caugner marked this pull request as draft October 20, 2022 10:24
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

This pull request has merge conflicts that must be resolved before it can be merged.

@caugner caugner changed the title perf: use GitHub API to get DIFF ci(pr-test): use GitHub API to get changed files + diff Dec 6, 2022
.github/workflows/pr-test.yml Show resolved Hide resolved
.github/workflows/pr-test.yml Show resolved Hide resolved
.github/workflows/pr-test.yml Show resolved Hide resolved
@yin1999
Copy link
Member Author

yin1999 commented Dec 7, 2022

A test workflow run is here: https://github.com/mdn/content/actions/runs/3635116130/jobs/6133911927

Also poke @bsmth for typo check :)

@yin1999 yin1999 marked this pull request as ready for review December 7, 2022 02:00
@yin1999 yin1999 requested a review from caugner December 7, 2022 03:05
.github/workflows/pr-test.yml Show resolved Hide resolved
.github/workflows/pr-test.yml Show resolved Hide resolved
.github/workflows/pr-test.yml Show resolved Hide resolved
@bsmth
Copy link
Member

bsmth commented Dec 7, 2022

Also poke @bsmth for typo check :)

I'm happy to take a look after the requested changes are applied. Please @ mention me when I should check 👀

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

This pull request has merge conflicts that must be resolved before it can be merged.

Comment on lines 148 to 150
- name: Validate redirects
if: ${{ env.GIT_DIFF_REDIRECTS }}
run: |
Copy link
Member Author

Choose a reason for hiding this comment

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

A bit confused here. We have redirect check workflow. So should we remove this part or remove that workflow?

Copy link
Contributor

Choose a reason for hiding this comment

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

The other call is slightly different, but I was also wondering about this call. I'd usually suggest the separate call, unless there actually is a benefit of running it after a build

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's keep it for now. The "redirect check" workflow is more strict, so in theory it should always also fail if this non-strict call (which just loads the file) fails. But I think it's a separate conversion, outside of the scope of this PR. 🙂

@caugner caugner merged commit 260e37e into mdn:main Dec 8, 2022
@yin1999 yin1999 deleted the speedup-pr-test branch December 8, 2022 10:54
hamishwillee pushed a commit to hamishwillee/content that referenced this pull request Dec 12, 2022
Also disables the workflow on the main branch,
as this workaround seems no longer necessary.
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.

4 participants