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

cmd/krel/cmd/ff: Initial commit of Go-based branchff tool #869

Merged
merged 1 commit into from Oct 31, 2019

Conversation

@justaugustus
Copy link
Member

justaugustus commented Sep 3, 2019

Introducing krel (the Kubernetes Release toolbox), and the first draft of the branch fast-forward subcommand, ff!

Here's the directory structure:

cmd/
├── krel
│   ├── BUILD.bazel
│   ├── cmd
│   │   ├── BUILD.bazel
│   │   ├── ff.go
│   │   └── root.go
│   └── main.go

pkg/
├──  util
    ├── BUILD.bazel
    ├── common.go
    └── git.go

After mulling this over for a bit, I decided we should get at least two things out of this:

  • A set of common libraries to use for the tooling rewrite (pkg/util/common.go and pkg/util/git.go are the first of these)
  • Unified CLI tool (via cobra), to take advantage of global flags (like --cleanup and --nomock) and common patterns

This is still pretty rough, but it's at least in a good place to merge the structure in and iterate! :)

Signed-off-by: Stephen Augustus saugustus@vmware.com

@justaugustus

This comment has been minimized.

Copy link
Member Author

justaugustus commented Sep 3, 2019

(...and yes, the finished product will have tests 😅 )

@justaugustus justaugustus force-pushed the justaugustus:bff branch 5 times, most recently from 0b14acd to ad43a85 Sep 8, 2019
@justaugustus justaugustus added this to In progress in Release Engineering Sep 23, 2019
@idealhack idealhack removed the request for review from claurence Sep 29, 2019
Copy link
Member

saschagrunert left a comment

Hey 👋, generally LGTM, but I have some comments. Regarding the many TODOs still left in the code I'd suggest to bring this in as soon as possible and then we can resolve them together. :)

@@ -0,0 +1,32 @@
linters-settings:

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 23, 2019

Member

I would suggest in adding golangci-lint as build target / verification script somewhere inside the CI.

}
*/

func AskYesOrNo(question string) (string, error) {

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 23, 2019

Member

I would return a bool here to make the usage more straight forward.

This comment has been minimized.

Copy link
@justaugustus

justaugustus Oct 31, 2019

Author Member

Done!

@@ -0,0 +1,203 @@
/*

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 23, 2019

Member

We may unify this file with the one from pkg/notes/git.go

This comment has been minimized.

Copy link
@hasheddan

hasheddan Oct 31, 2019

Contributor

Agreed here, git related things feel like they would be worthy of their own package

This comment has been minimized.

Copy link
@justaugustus

justaugustus Oct 31, 2019

Author Member

+1! That was exactly what I had in mind. :)


func GetMergeBase(masterRefShort, releaseRefShort string, repo *git.Repository) (string, error) {
masterRef := fmt.Sprintf("origin/%s", masterRefShort)
releaseRef := fmt.Sprintf("origin/%s", releaseRefShort)

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 23, 2019

Member

The default remote origin may be a const value somewhere since it's used in many places.

This comment has been minimized.

Copy link
@justaugustus

justaugustus Oct 31, 2019

Author Member

Fixed!

}

if len(res) == 0 {
os.Exit(1)

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 23, 2019

Member

I suggest exit'ing in the main path and return an error here instead.

This comment has been minimized.

Copy link
@justaugustus

justaugustus Oct 31, 2019

Author Member

Fixed!

log.Println(err)
os.Exit(1)
Comment on lines 71 to 72

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 23, 2019

Member
Suggested change
log.Println(err)
os.Exit(1)
log.Fatal(err)

This comment has been minimized.

Copy link
@justaugustus

justaugustus Oct 31, 2019

Author Member

Fixed!

@justaugustus

This comment has been minimized.

Copy link
Member Author

justaugustus commented Oct 28, 2019

@saschagrunert -- sounds like a plan! I'll work on this this week.

@justaugustus

This comment has been minimized.

Copy link
Member Author

justaugustus commented Oct 29, 2019

/test pull-release-test

@justaugustus justaugustus force-pushed the justaugustus:bff branch from 907555b to 8f416a3 Oct 31, 2019
@justaugustus justaugustus changed the title [WIP] cmd/branchff: Initial commit of Go-based branchff tool cmd/krel/cmd/ff: Initial commit of Go-based branchff tool Oct 31, 2019
@justaugustus

This comment has been minimized.

Copy link
Member Author

justaugustus commented Oct 31, 2019

/assign @saschagrunert @hasheddan
/milestone v1.17

@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 31, 2019
@justaugustus justaugustus force-pushed the justaugustus:bff branch from 8f416a3 to 4308cdf Oct 31, 2019
Copy link
Contributor

hasheddan left a comment

This is awesome @justaugustus, thanks for leading this effort :) A few comments below. Not sure at what state we should deem this mergeable, but I am fine moving forward with something that isn't perfect given we intend to iterate on this more.

)

// Run wraps the exec.Cmd.Run() command and sets the standard output.
// TODO: Should this take an error code argument/return an error code?

This comment has been minimized.

Copy link
@hasheddan

hasheddan Oct 31, 2019

Contributor

This a great thing to point out and I definitely think Run() should be returning error here. Otherwise, any user of this package is unable to determine whether to exit or continue as they see fit based on the outcome of calling this function. While it doesn't make a huge difference for ff, it could prove difficult to work with if any other tool consumes it.

@@ -0,0 +1,203 @@
/*

This comment has been minimized.

Copy link
@hasheddan

hasheddan Oct 31, 2019

Contributor

Agreed here, git related things feel like they would be worthy of their own package

// We can then use every Remote functions to retrieve wanted information
refs, err := rem.List(&git.ListOptions{})
if err != nil {
log.Fatal(err)

This comment has been minimized.

Copy link
@hasheddan

hasheddan Oct 31, 2019

Contributor

Would be useful to return an informative error here.

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 31, 2019

Member

Later: We could use something like wrapping errors with additional context to provide some sort of error stack.

*/

func Ask(question, expectedResponse string) (string, bool, error) {
retries := 3

This comment has been minimized.

Copy link
@hasheddan

hasheddan Oct 31, 2019

Contributor

How would you feel about making retries a parameter here?

}

log.Printf(
`Go look around in %s to make sure things look okay before pushing...

This comment has been minimized.

Copy link
@hasheddan

hasheddan Oct 31, 2019

Contributor

nitpick: it might be more readable to move this to a separate variable outside the main function execution

@@ -0,0 +1,32 @@
load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 31, 2019

Member

I’m wondering if we need bazel at all if we stick to go modules and a recent version (>1.12).

This comment has been minimized.

Copy link
@justaugustus

justaugustus Oct 31, 2019

Author Member

@saschagrunert -- Though I grumble on occasion about bazel, we get the advantage of leveraging the verify tests from k/repo-infra.

Updating & verifying all things is now simple:

  • update: ./hack/update-all.sh
  • verify: ./hack/verify-all.sh

See here: #889, #890

This comment has been minimized.

Copy link
@hoegaarden

hoegaarden Nov 1, 2019

Member

It, being the verify, does not seem to work for me: #908

It might well be an issue on my side, could someone maybe check if that actually works for you?

dryRunFlag = "--dry-run"
}

re := regexp.MustCompile(util.BranchRE)

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 31, 2019

Member

I would add this as a stand alone function to the utils and avoid making BranchRE public, like: util.IsReleaseBranch(string)

log.Fatalf("%s is not a release branch\n", branch)
}

branchExists, err := util.BranchExists(branch)

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 31, 2019

Member

Can we return either a bool or an error here? Because the value of err is unhandled and it could be that we return nil below.

This comment has been minimized.

Copy link
@justaugustus

justaugustus Oct 31, 2019

Author Member

Still returning both, but the error is handled now.

defer cleanupTmpDir(baseDir)
}

workingDir := fmt.Sprintf("%s/%s", baseDir, branch)

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 31, 2019

Member

I would stick to filepath.Join for path manipulations.


currentPath := fmt.Sprintf("%s", os.Getenv("PATH"))
etcdPath := fmt.Sprintf("%s/third_party/etcd:%s", gitRoot, currentPath)
os.Setenv("PATH", os.Getenv("PATH"))

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 31, 2019

Member

We should set it to etcdPath here, right?

)

// rootCmd represents the base command when called without any subcommands
var rootCmd = &cobra.Command{

This comment has been minimized.

Copy link
@saschagrunert

saschagrunert Oct 31, 2019

Member

This is more a question to which style we want to stick:

Do we want to avoid global variables and init functions by moving them into main?

This comment has been minimized.

Copy link
@justaugustus

justaugustus Oct 31, 2019

Author Member

@saschagrunert -- I think I like the layout for this iteration, but we can talk style more once this merges. :)

Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@justaugustus justaugustus force-pushed the justaugustus:bff branch from 4308cdf to ca87ff2 Oct 31, 2019
@justaugustus

This comment has been minimized.

Copy link
Member Author

justaugustus commented Oct 31, 2019

@hasheddan @saschagrunert -- I've addressed all of your comments, so this should be good to approve/merge now!

Copy link
Contributor

hasheddan left a comment

Assuming we are good with deferring tests to a future iteration (which seems fine considering we don't plan to be using this in a production setting until the tool is more robust) then LGTM!

/lgtm /approve

@justaugustus

This comment has been minimized.

Copy link
Member Author

justaugustus commented Oct 31, 2019

@hasheddan -- As a heads up, /lgtm and /approve need to be on separate lines as the first and only thing on the line, like so:

/lgtm
/approve
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 31, 2019

@justaugustus: you cannot LGTM your own PR.

In response to this:

@hasheddan -- As a heads up, /lgtm and /approve need to be on separate lines as the first and only thing on the line, like so:

/lgtm
/approve

Instructions 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.

@justaugustus

This comment has been minimized.

Copy link
Member Author

justaugustus commented Oct 31, 2019

(haha, I knew it was going to say that.)
/honk

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 31, 2019

@justaugustus:
goose image

In response to this:

(haha, I knew it was going to say that.)
/honk

Instructions 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.

@hasheddan

This comment has been minimized.

Copy link
Contributor

hasheddan commented Oct 31, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 31, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hasheddan, justaugustus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 82366a4 into kubernetes:master Oct 31, 2019
3 of 4 checks passed
3 of 4 checks passed
tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation justaugustus authorized
Details
pull-release-cluster-up Job succeeded.
Details
pull-release-test Job succeeded.
Details
Release Engineering automation moved this from In progress to Done (1.17) Oct 31, 2019
Copy link
Member

hoegaarden left a comment

While I really appreciate all the work and thoughts that are going into re-writing the tools in go, I am abit concerned about the general direction:

This seems to be generally go int o a direction of a monolithic tool for all the release things. I think it would be more useful to break the release tools apart. What do I mean by that:
In short: don't implement things & "wiring" that other tools can do for us, just implement our specifics. Specifically In the case of branchff: really only implement our fast forwarding and merging business, push and pull and other stuff should be done elsewhere.

Why?

  • We can leverage other tools (GCB) to wire the things together
  • It's easier to change / adapt / ... e.g. we can just run other steps before/after the thing (branchff in this case) and the thing (branchff in this case) doesn't need to know or change

Consider this:

# "monostep" / bakcbox-ish step
- name: gcr.io/k8s-release-tools/krel
  args: [ "branchff", "--some-arg" ]

vs.

# get a thing, maybe not even git.k8s.io/kubernetes but some downstream vendor's repo
- name: gcr.io/cloud-builders/git
  dir: src/k8s
  args: [ "pull", "$_K8S_RPEO", "--branch", "$_K8S_REPO" ]
# only do the FF, nothing else 
- name: gcr.io/k8s-release-tools/branchff
  dir: src.k8s
  args: [ "$_FF_BRANCH" ]
# push it upstream, wherever that is
- name:  gcr.io/cloud-builders/git
  dir:  src/k8s
  args: [ "push", "upstream", "$_FF_BRANCH" ]

Now I want to run some tests on the branchff'ed branch (e.g. run e2e on kind or whatnot). Ideally that would be just adding a new GCB task, and not changing any code in branchff/krel/...

   [....]
# only do the FF, nothing else 
- name: gcr.io/k8s-release-tools/branchff
  dir: src.k8s
  args: [ "$_FF_BRANCH" ]
# run some tests with the branchff'ed code
- name: gcr.io/k8s-release-tools/kind-e2e
  args: [ "--k8s-src", "src/k8s" ]
# push it upstream, wherever that is -- IFF the last step succeeded
- name:  gcr.io/cloud-builders/git
  dir:  src/k8s
  args: [ "push", "upstream", "$_FF_BRANCH" ]

To summarise:

  • I am not saying the idea of krel is bad, I am merely questioning the direction it seems to go a bit.
  • I am concerned we might be on a path were we pack too much responsibility & wiring into the tool, and not leveraging enough of the other tools we are using already anyway.
  • I'd rather have some small sharp tools with a defined interface between each other. In the case of GCB the interface between multiple steps are directories to be consumed and produced. That's an easy enough thing to
    • run/wire locally
    • extract, reshuffle, extend, run single, ... steps
    • move to another system somewhen (e.g. if we find we need tekton).
@@ -0,0 +1,32 @@
load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")

This comment has been minimized.

Copy link
@hoegaarden

hoegaarden Nov 1, 2019

Member

It, being the verify, does not seem to work for me: #908

It might well be an issue on my side, could someone maybe check if that actually works for you?

@justaugustus

This comment has been minimized.

Copy link
Member Author

justaugustus commented Nov 1, 2019

We're chatting in Slack (https://kubernetes.slack.com/archives/CJH2GBF7Y/p1572605765115500) w.r.t. @hoegaarden's review.
I'm going to drop notes later on the umbrella issue to talk more about what's in my head direction-wise.

@justaugustus justaugustus mentioned this pull request Nov 1, 2019
6 of 15 tasks complete
@tpepper

This comment has been minimized.

Copy link
Contributor

tpepper commented Nov 1, 2019

I think @hoegaarden 's concerns are valid. As new things are built there definitely should also be refactoring common things out of existing tools and instituting new splits in the tools and flow (as in the pull, branchff, push example). Those are more testable. It's totally conceivable one person wont see a pattern and other reviewers will, and ask for a refactor. We'll have to decide on a case by case basis if the refactor should happen before merge, be done by the original change proposer, or if it can follow and if somebody else can do it. The latter path will likely lead to more merge conflicts and rebase issues if folks aren't communicating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.