Skip to content

Don't getCiBuildId from GitHub when it's already overridden in the action parameters#550

Merged
admah merged 12 commits intocypress-io:masterfrom
jdborneman-terminus:master
May 20, 2022
Merged

Don't getCiBuildId from GitHub when it's already overridden in the action parameters#550
admah merged 12 commits intocypress-io:masterfrom
jdborneman-terminus:master

Conversation

@jdborneman-terminus
Copy link
Copy Markdown
Contributor

@jdborneman-terminus jdborneman-terminus commented May 18, 2022

We're doing Strange Things™️ with our setup. Long story short we have an infra repo that deploys and tests a service repo. The service repo is the one that contains the tests. Instead of having the infra repo call out to a workflow in the service repo (causing all sorts of interdependency between workflows) we've instead wrapped the cypress-io/github-action in a custom composite action that gets run from teh Infra repo and pulls down the service repo, testing it as part of teh infra workflow.

BUT... it fails in the following way:

2022-05-18T18:24:10.078Z @cypress/github-action no need to verify Cypress binary or save caches
2022-05-18T18:24:10.078Z @cypress/github-action No start command found
2022-05-18T18:24:10.079Z @cypress/github-action Running Cypress tests using CLI command
2022-05-18T18:24:10.079Z @cypress/github-action with concatenated command prefix: yarn
2022-05-18T18:24:10.079Z @cypress/github-action Determining build id by asking GitHub about run 2347025404
2022-05-18T18:24:10.159Z @cypress/github-action Not Found
2022-05-18T18:24:10.166Z @cypress/github-action HttpError: Not Found
Error: Not Found
    at /home/runner/work/_actions/cypress-io/github-action/v2/dist/index.js:58740:21
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async getCiBuildId (/home/runner/work/_actions/cypress-io/github-action/v2/dist/index.js:6230:18)
    at async runTestsUsingCommandLine (/home/runner/work/_actions/cypress-io/github-action/v2/dist/index.js:6347:33)

I was able to track down that it's failing because it's trying to determine a build id from the repo but I believe it's confused about which repo and run id to use since we're inside a repo-in-a-repo.

This change just make sit so that getCiBuildId isn't called if the ci-build-id parameter is already provided.

Note: Not sure why the package-lock.json got changed so drastically. All I did was an npm install.

Anyway open to feedback and changes to what I did as js is not my best language (yet 😁 ).

@jdborneman-terminus
Copy link
Copy Markdown
Contributor Author

jdborneman-terminus commented May 18, 2022

jdborneman-terminus#1

A PR I did on my fork showed 51 of 52 checks passed. Not sure why the 1 failed TBH:
https://github.com/jdborneman-terminus/cypress-io-github-action/runs/6495534617?check_suite_focus=true

admah
admah suggested changes May 19, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@admah admah left a comment

Choose a reason for hiding this comment

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

See a couple of comments. Thanks!

Comment thread index.js Outdated
Comment thread package-lock.json
@jdborneman-terminus
Copy link
Copy Markdown
Contributor Author

jdborneman-terminus commented May 19, 2022

@admah So .... strange things. I reverted package-lock.json, but now when I do an npm run build it shows a LOT more changes to dist/index.js than I expected. I'm currently testing it but if you've got any advice as to WHY this might be happening I'm all ears. I'm on npm version 6.14.15

@jdborneman-terminus
Copy link
Copy Markdown
Contributor Author

jdborneman-terminus commented May 20, 2022

@admah I upgraded npm to 8* and it's still makign pretty large changes to dist/index.js when I npm run build (although it now doesn't monkey with the package-lock when I npm install at least).

@admah
Copy link
Copy Markdown
Contributor

admah commented May 20, 2022 •
edited
Loading

@jdborneman-terminus I'm taking a look now to see what the diff is.

UPDATE: after looking at it on my own branch and building, there are dynamic changes made to the dist file. Since all the checks and tests pass, I'm approving and merging.

@admah admah merged commit e102825 into cypress-io:master May 20, 2022
@jdborneman-terminus
Copy link
Copy Markdown
Contributor Author

jdborneman-terminus commented May 23, 2022

Thanks @admah! Happy to be able to contribute. So when will it be tagged/released? Is that something I can/should do, or leave it to you all?

@admah
Copy link
Copy Markdown
Contributor

admah commented May 23, 2022

@jdborneman-terminus I'm working on this. We follow semantic-release commit conventions and I missed that no commits seem to have feat:. That means no release was kicked off. I'll update this once it's updated. Thanks!

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 23, 2022

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@admah admah admah approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jdborneman-terminus @admah