-
Notifications
You must be signed in to change notification settings - Fork 3
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
amplify-preview
: custom action to trigger amplify previews
#287
Conversation
844b294
to
31fd385
Compare
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.
IMO this should be written as a Go or typescript tool. We've been trying to move this direction to reduce our reliance on GHA.
If you decide to go this route, there are tools in this directory for CI/CD workflows as well as local tooling.
This comment was marked as outdated.
This comment was marked as outdated.
5e63c93
to
31b9ce9
Compare
f89122e
to
ac7989e
Compare
4578f7d
to
5e7aaf5
Compare
5e7aaf5
to
9cec7a5
Compare
gopkg.in/yaml.v3 v3.0.1 // indirect | ||
) | ||
|
||
replace github.com/gravitational/shared-workflows/libs => ../../libs/ |
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 needed to use new functions added to the github
package.
Can be removed once new version is released.
92d7532
to
5b33a39
Compare
9b2b5f0
to
b7d7899
Compare
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.
👍 First pass.
WAIT: ${{ inputs.wait }} | ||
shell: bash | ||
run: | | ||
pushd ./tools/amplify-preview/; go run ./; popd |
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.
Do we need shell and pushd? go run ./tools/amplify-preview
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 can be removed in subsequent release PR once libs
package is released and I can remove the replace
rule from go.mod:
replace github.com/gravitational/shared-workflows/libs => ../../libs/ |
"strings" | ||
"time" | ||
|
||
"github.com/alecthomas/kingpin/v2" |
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.
For consistency, let's use either the flag package or cobra.
E.g.
Lines 118 to 131 in 0f1f9de
func parseFlags() (flags, error) { | |
var ( | |
workflow = flag.String("workflow", "", "specific workflow to run [assign, check, dismiss, backport]") | |
token = flag.String("token", "", "GitHub authentication token") | |
reviewers = flag.String("reviewers", "", "reviewer assignments") | |
local = flag.Bool("local", false, "local workflow dry run") | |
org = flag.String("org", "", "GitHub organization (local mode only)") | |
repo = flag.String("repo", "", "GitHub repository (local mode only)") | |
prNumber = flag.Int("pr", 0, "GitHub pull request number (local mode only)") | |
branch = flag.String("branch", "", "GitHub backport branch name (local mode only)") | |
baseStats = flag.String("base", "", "the artifact sizes as generated by binary-sizes to compare against for bloat") | |
buildDir = flag.String("builddir", "", "an absolute path to a build directory containing artifacts to be checked for bloat") | |
artifacts = flag.String("artifacts", "", "a comma separated list of compile artifacts to analyze for bloat") | |
) |
https://github.com/gravitational/cloud/blob/master/tcctl/release/cli/cli.go
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.
All tools in https://github.com/gravitational/shared-workflows/tree/master/tools are using github.com/alecthomas/kingpin
and I followed their example for consistency 😬
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.
Not sure why kingpin was chosen for those tools. The review bot uses the simple flag package. We previously used kingpin in cloud but changed to cobra because it has a better design.
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 think it's because teleport uses kingpin everywhere:
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.
Changing to align with other cloud projects is up to you. Lots of big projects chose cobra (K8s, Hugo, Github CLI, Docker, Istio, Helm, etc).
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 would prefer to keep it as is to avoid spending more time on this, as long term we would prefer using native integration, once Amplify get to resolve current issues.
Co-authored-by: Jim Bishopp <[email protected]>
0d00076
to
877547e
Compare
877547e
to
42c033c
Compare
for _, c := range comments { | ||
matcher := true | ||
if targetComment.UserLogin != "" { | ||
matcher = matcher && c.User.GetLogin() == targetComment.UserLogin | ||
} | ||
|
||
if targetComment.BodyContains != "" { | ||
matcher = matcher && strings.Contains(c.GetBody(), targetComment.BodyContains) | ||
} | ||
|
||
if matcher { | ||
return c, nil | ||
} | ||
} |
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.
Proper zero values for CommentTraits means you can remove matcher and the empty string checks. Also, see https://pkg.go.dev/slices#ContainsFunc
"strings" | ||
"time" | ||
|
||
"github.com/alecthomas/kingpin/v2" |
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.
Changing to align with other cloud projects is up to you. Lots of big projects chose cobra (K8s, Hugo, Github CLI, Docker, Istio, Helm, etc).
failedResp := aggregatedError{ | ||
perAppErr: map[string]error{}, | ||
message: "failed to fetch branch", | ||
} |
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 aggregatedError needed? Looks like its value is in deduplicating error messages when the same appID is passed in via a flag. Why not deduplicate in main before reaching this point? That way this doesn't make two calls to GetBranch for the same appID and you could gather errors using a slice with errors join.
Same thing applies for CreateBranch.
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.
The original purpose was not really to deduplicate errors but just to associate error with app where it happened, and then combine in one aggregated error.
8ec9db3
to
ce44d5b
Compare
ce44d5b
to
31cce17
Compare
Summary
This PR creates custom github action which:
create-branch
enabledwait
enabledContext
Because of limitations with AWS Amplify, we can't use Amplify's GitHub integration in
teleport
ordocs-website
repos:AMPLIFY_DIFF_DEPLOY
aws-amplify/amplify-hosting#3959Testing
Tested on:
This is a more advanced version of workflow running in
docs-website
repo, once this is merged, I will configuredocs-website
to run this version:Run example:
PR comment looks like following:
or if latest deployment is not successful, it will show latest deployment and latest successful deployment.
Footnotes
https://docs.aws.amazon.com/general/latest/gr/amplify.html ↩