-
Notifications
You must be signed in to change notification settings - Fork 32
Add GA #144
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 GA #144
Conversation
c018ab7 to
6230304
Compare
| branches: [main] | ||
| pull_request: | ||
| branches: | ||
| - main |
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.
Is this right? Will this upload the artifact from PRs before they are merged?
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 assume you don't want to do that)
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.
No I don't, this is all still WIP testing phase. I need devprod to create some credentials first.
b67b098 to
901ad4f
Compare
901ad4f to
ce2a742
Compare
| branches: [main] | ||
| pull_request: | ||
| branches: | ||
| - main |
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.
Trying out all the YAML list variations, huh?
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.
LOL I don't understand yaml to this day.
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.
Isn't this one the normal one?
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.
This is the most common one I think but right above for identical branches you have the other one.
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.
just to clarify:
push.branches. triggers it whenever a commit gets pushed to the given list of branches. so this would trigger when a PR is merged to themainbranch, and also if someone pushed directly to it (though we disallow that via branch protection rules)pull_request.branches. triggers the job when a PR against themainbranch is opened, or when new commits to an existing PR's branch are pushedworkflow_dispatch. allows the workflow to be triggered by other workflows or manually when a person runs it from the web UI.
so your goal is to have this workflow run in any of the above scenarios?
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.
Yes the first two is what we want.
The last I don't really mind but is good to have I guess.
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: |
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.
Cool, so we get build on PRs too, nice!
| run: mvn clean compile package -DskipTests | ||
|
|
||
| - name: Upload to S3 bucket | ||
| if: github.event_name == 'push' |
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.
So this works on a GH "merge PR" click, right?
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.
That's what I expect yes.
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.
so this will ONLY upload when a PR is merged. that's what you want, right?
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.
Yes exactly.
travisdowns
left a comment
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, but let's get devprod eyes on it too.
|
lgtm |
Adds a github actions job to upload the minimal tar.gz to our S3 bucket
such that we can use it in bazel.