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

fix: trigger pipeline for non main branches #11

Merged
merged 4 commits into from
Mar 8, 2022

Conversation

mattlewis92
Copy link
Contributor

@mattlewis92 mattlewis92 commented Feb 17, 2022

Hey there! I decided to give this action a go based on this blog post but I found that it kept trying to trigger the pipeline on the main branch rather than the branch which triggered the job. After a bit of digging I found the issue was that the tag and branch aren't being passed correctly to the circleci pipeline according to the docs

Also included several other fixes to make contributing easier (done as separate commits for a cleaner git history)

.DS_Store
.idea
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this for webstorm users

@@ -23,7 +23,7 @@ jobs:
steps:
- name: <customize name>
id: <customize id>
uses: CircleCI-Public/trigger-circleci-pipeline-action@v1.0
uses: CircleCI-Public/trigger-circleci-pipeline-action@v1.0.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I copy pasted this I realised I wasn't on the latest version of the action. You may also want to maintain a v1 tag and update that on every release.

@@ -18,13 +18,13 @@ info(`Org: ${repoOrg}`);
info(`Repo: ${repoName}`);
const ref = context.ref;

const branch = () => {
const getBranch = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed these so the intent is a bit clearer. I think if you were doing if (getTag) { it would have been more obvious there was a bug here

if (tag) {
Object.assign(parameters, { GHA_Tag: tag() });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GHA_Tag and GHA_Branch were not documented in the readme and if they were set would have caused the circle build to fail as they wouldn't have been defined on the circle config. It doesn't make sense to pass them as parameters as they can be determined from within any job inside of circleci so I just removed them

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct! I was trying to figure out why this code would be here (my fault obviously), and I think I found the broken train of thought. I think the original intent may have been to append "branch" and "tag" to the body.

You are correct we would not need either of those parameters, which led me to question why I put them there. I think the intent was the ensure the pipeline was triggered on a branch or tag, and it appears that is likely not working now.

https://circleci.com/docs/api/v2/#operation/triggerPipeline

Payload sample:

{
  "branch": "feature/design-new-api",
  "tag": "v3.1.4159",
  "parameters": {
    "deploy_prod": true
  }
}

Rather than remove them I think we might want to make a few adjustments to ensure the branch/tag key are added to the body so that the pipeline is triggered at the right reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure! That's what we're now doing here (and was the main source of the bug):

if (tag) {
Object.assign(body, { tag });
} else {
Object.assign(body, { branch });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on how we check the ref field it looks like it's not possible to have both a tag AND a branch so I think it's correct to only pass one or the other

if (ref.startsWith("refs/heads/")) {

if (ref.startsWith("refs/tags/")) {

@@ -51,16 +51,24 @@ const body = {
parameters: parameters,
};

const tag = getTag();
const branch = getBranch();

if (tag) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this was always truthy as we were referencing the function rather than its return value.

@@ -24,11 +24,10 @@
"dependencies": {
"@actions/core": "^1.6.0",
"@actions/github": "^5.0.0",
"axios": "^0.24.0",
"install": "^0.13.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these 2 were added as accidental dependencies

},
"devDependencies": {
"@vercel/ncc": "^0.33.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ncc as a dev dependency so things just work out the box without having to install it globally

@rowi1de
Copy link

rowi1de commented Mar 4, 2022

Looks like this #12 or?

@mattlewis92
Copy link
Contributor Author

Looks like this #12 or?

I think we're running into the same problem here, where the action always triggers the main branch's pipeline, this PR fixes it so it only triggers the circle pipeline for the branch or tag that triggered the github action

@mattlewis92
Copy link
Contributor Author

@KyleTryon any update on if we can get this merged soon? 😄

@KyleTryon KyleTryon merged commit 7b29cee into CircleCI-Public:main Mar 8, 2022
@KyleTryon
Copy link
Contributor

Apologies for the delay @mattlewis92. I reviewed the code, rebuilt it to verify and as there were no changes, immediately tagged your change.
https://github.com/CircleCI-Public/trigger-circleci-pipeline-action/releases/tag/v1.0.4

🙇 thank you

@mattlewis92
Copy link
Contributor Author

Awesome, thank you so much!! 🙌

@milosivanovic
Copy link

milosivanovic commented Apr 1, 2022

Currently, GitHub references the default/main branch instead of the branch the PR was created from for all issue_comment events.

I'm trying to create a comment trigger so if I type something like /deploy in a PR comment, GitHub Actions trigger a CircleCI build of the PR. As seen in the GitHub documentation, issue_comment refs are refs/heads/<default_branch> instead of the PR branch, so this always builds the main branch, even with @mattlewis92's fix.

There are a few complaints regarding this here: actions/checkout#331 (comment)

Is there any way to make issue_comment events trigger the right (non-main) branch in trigger-circleci-pipline-action?

zackse added a commit to zackse/trigger-circleci-pipeline-action that referenced this pull request Apr 7, 2022
It may be preferable to set a v1 link and keep that updated to point to
the latest release.

Also mentioned in PR CircleCI-Public#11:
CircleCI-Public#11 (comment)
@joshS28
Copy link

joshS28 commented May 19, 2022

Same problem here with @milosivanovic .

Experiencing the same problem myself and have left a comment here actions/checkout#331 trying to see if there exists a solution.

It seems like CircleCI is not picking up the branch correctly, is there an example of passing the branch as a parameter?

KyleTryon pushed a commit that referenced this pull request Jun 17, 2022
* support events that don't have a payload

For example, 'on: schedule' does not have a payload[1], which results in
an error when attempting to use this action:

    TypeError: Cannot read property 'url' of undefined

This commit updates the code to use the `repo` attribute instead
of parsing out the repo org/name from the repo URL.

1. https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule

* prepare build

```
npm run lint && npm run format && npm run build
```

Added this as a separate commit, as I don't know anything about npm
packages, and it's not clear if all of the package list changes are
desirable.

* README: unify version

It may be preferable to set a v1 link and keep that updated to point to
the latest release.

Also mentioned in PR #11:
#11 (comment)
omxela pushed a commit to mo-work/trigger-circleci-pipeline-action that referenced this pull request Oct 9, 2022
* support events that don't have a payload

For example, 'on: schedule' does not have a payload[1], which results in
an error when attempting to use this action:

    TypeError: Cannot read property 'url' of undefined

This commit updates the code to use the `repo` attribute instead
of parsing out the repo org/name from the repo URL.

1. https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule

* prepare build

```
npm run lint && npm run format && npm run build
```

Added this as a separate commit, as I don't know anything about npm
packages, and it's not clear if all of the package list changes are
desirable.

* README: unify version

It may be preferable to set a v1 link and keep that updated to point to
the latest release.

Also mentioned in PR CircleCI-Public#11:
CircleCI-Public#11 (comment)
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.

5 participants