Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @ibix16. Thanks for your PR. I'm waiting for a aws member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
eks-distro-bot
added
needs-ok-to-test
size/XXL
|
/ok-to-test |
| version: 0.2 | ||
| env: | ||
| secrets-manager: | ||
| SECRET_PAT: "Secret:PAT" |
There was a problem hiding this comment.
Let's change this secret to an existing one in the build account
| pre_build: | ||
| commands: | ||
| - echo "Downloading compiled binary..." | ||
| - aws s3 cp s3://eka-a-releaser-build-output/eks-a-releaser-build . |
There was a problem hiding this comment.
Let's make the s3 bucket name configurable
ibix16
force-pushed
the
eks-a-releaser-push
branch
from
8e24aad to
126b86f
Compare
ibix16
force-pushed
the
eks-a-releaser-push
branch
from
126b86f to
e649269
Compare
|
Let's update the description section with details about the PR and testing. Also this is a very large PR to review. We might end up missing things while reviewing. Can we break these PRs up into smaller PRs? |
release/cli/cmd/create-branch.go
Outdated
| if RELEASE_TYPE == "minor" { | ||
| err := createMinorBranches() | ||
| if err != nil { | ||
| fmt.Printf("error calling createMinorBranches %s", err) |
There was a problem hiding this comment.
Let's return these errors to the caller instead of using Printf statements throughout.
release/cli/cmd/create-branch.go
Outdated
| // else | ||
| err := createPatchBranch() | ||
| if err != nil { | ||
| fmt.Printf("error calling createPatchBranch %s", err) |
There was a problem hiding this comment.
Same comment about printing errors.
release/cli/cmd/create-release.go
Outdated
|
|
||
| // else | ||
| //create client | ||
| accessToken := os.Getenv("SECRET_PAT") |
There was a problem hiding this comment.
nit: we can define this as a constant instead of writing the string value at multiple places.
release/cli/cmd/create-release.go
Outdated
| return rel, nil | ||
| } | ||
|
|
||
| func retrieveLatestProdCLIHash(releaseType string) string { |
There was a problem hiding this comment.
| func retrieveLatestProdCLIHash(releaseType string) string { | |
| func retrieveLatestProdCLIHash(releaseType string) error { |
Let's return error instead of the string
release/cli/cmd/create-release.go
Outdated
|
|
||
| commits, _, err := client.Repositories.ListCommits(ctx, usersForkedRepoAccount, EKSAnyrepoName, opts) | ||
| if err != nil { | ||
| return "error fetching commits list" |
There was a problem hiding this comment.
| return "error fetching commits list" | |
| return fmt.Errorf("fetching commits list %v", err) |
something like this
release/cli/cmd/create-release.go
Outdated
|
|
||
| func createGitHubRelease(releaseTag *github.RepositoryRelease) (*github.RepositoryRelease, error) { | ||
|
|
||
| latestVersionValue := os.Getenv("LATEST_VERSION") |
There was a problem hiding this comment.
nit: constant for LATEST_VERSION
release/cli/cmd/create-release.go
Outdated
| ctx := context.Background() | ||
| client := github.NewClient(nil).WithAuthToken(accessToken) | ||
|
|
||
| release, _, err := client.Repositories.GetReleaseByTag(ctx, upStreamRepoOwner, EKSAnyrepoName, latestVersionValue) |
There was a problem hiding this comment.
Shouldn't we also return here if we encounter an error?
| release, _, err := client.Repositories.GetReleaseByTag(ctx, upStreamRepoOwner, EKSAnyrepoName, latestVersionValue) | |
| release, _, err := client.Repositories.GetReleaseByTag(ctx, upStreamRepoOwner, EKSAnyrepoName, latestVersionValue) | |
| if err != nil { | |
| return nil, err | |
| } |
release/cli/cmd/create-release.go
Outdated
|
|
||
| release, _, err := client.Repositories.GetReleaseByTag(ctx, upStreamRepoOwner, EKSAnyrepoName, latestVersionValue) | ||
| if err == nil { | ||
| fmt.Printf("Release %s already exists!\n", latestVersionValue) |
There was a problem hiding this comment.
I would recommend logging these statements rather than printing them.
|
|
||
| pr, _, err := client.PullRequests.Create(ctx, upStreamRepoOwner, EKSAnyrepoName, newPR) | ||
| if err != nil { | ||
| return fmt.Errorf("error creating PR %s", err) |
There was a problem hiding this comment.
| return fmt.Errorf("error creating PR %s", err) | |
| return fmt.Errorf("creating PR %v", err) |
4 participants
Issue #, if available:
Description of changes:
Testing (if applicable):
Documentation added/planned (if applicable):
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.