-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Change the order of flags in github actions workflows. #1906
Conversation
There was a problem hiding this 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. Thank you
@weppos I think we'll need this PR in soon so we can keep up with the tld-update workflow. |
Why is there a rush?
…On Mon, Nov 27, 2023, 11:46 AM Amir Omidi ***@***.***> wrote:
@weppos <https://github.com/weppos> I think we'll need this PR in soon so
we can keep up with the tld-update workflow.
—
Reply to this email directly, view it on GitHub
<#1906 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACQTJLHNIVVX4HUBRM7FNLYGTU2NAVCNFSM6AAAAAA74QAFH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRYGQ4TKNJQHA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
I accidentally introduced a bug in #1901 to the github worklow: list/.github/workflows/tld-update.yml Line 29 in 2d1427a
This caused the last tld-update run to fail: https://github.com/publicsuffix/list/actions/runs/7006982080/job/19060050437 This PR fixes that so we don't have this failure on the next run. |
Ah, ok I was wondering why Comcast's TLDs didn't get removed.
…On Mon, Nov 27, 2023, 2:03 PM Amir Omidi ***@***.***> wrote:
I accidentally introduced a bug in #1901
<#1901> to the github worklow:
https://github.com/publicsuffix/list/blob/2d1427a99ee0c3eef8b22d06c4d7099200ed41ac/.github/workflows/tld-update.yml#L29
This caused the last tld-update run to fail:
https://github.com/publicsuffix/list/actions/runs/7006982080/job/19060050437
This PR fixes that so we don't have this failure on the next run.
—
Reply to this email directly, view it on GitHub
<#1906 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACQTJPRZU32RSBPVUVZI63YGUE4NAVCNFSM6AAAAAA74QAFH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRYGY4TKNZTGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
@@ -18,7 +18,7 @@ jobs: | |||
go-version: 'stable' | |||
|
|||
- name: Run Go unit tests | |||
run: go test -C ./tools . | |||
run: go test -C ./tools -v . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, any reason to not have:
run: go test -C ./tools -v . | |
run: cd ./tools && go test -v ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of chaining a cd
, what about using the working-directory job config element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on how this is done. Up to maintainers to decide this.
I do have a preference for using the tools that Go has built in to the toolchain.
My main opinion is this is currently breaking the pipeline, so either we rollback #1901, or we accept this as is and revisit it if the need arises?
Mistake introduced in #1901
Fixes https://github.com/publicsuffix/list/actions/runs/7006982080/job/19060050437