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

push build: initial migration to krel #941

Merged
merged 1 commit into from
Dec 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ filegroup(
"//pkg/command:all-srcs",
"//pkg/git:all-srcs",
"//pkg/notes:all-srcs",
"//pkg/release:all-srcs",
"//pkg/util:all-srcs",
],
tags = ["automanaged"],
Expand Down
2 changes: 2 additions & 0 deletions cmd/krel/cmd/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/git:go_default_library",
"//pkg/release:go_default_library",
"//pkg/util:go_default_library",
"@com_github_pkg_errors//:go_default_library",
"@com_github_spf13_cobra//:go_default_library",
"@com_google_cloud_go//storage:go_default_library",
],
)

Expand Down
102 changes: 96 additions & 6 deletions cmd/krel/cmd/push.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,15 @@ limitations under the License.
package cmd

import (
"context"
"log"
"os"
"os/user"

"cloud.google.com/go/storage"
"github.com/spf13/cobra"

"k8s.io/release/pkg/release"
)

const description = `
Expand Down Expand Up @@ -125,16 +131,16 @@ func init() {
)
pushBuildCmd.PersistentFlags().StringVar(
&pushBuildOpts.releaseKind,
"release-kind",
"devel",
"Specify an alternate bucket for pushes (normally 'devel' or 'ci')",
)
pushBuildCmd.PersistentFlags().StringVar(
&pushBuildOpts.releaseType,
"release-type",
"kubernetes",
"Kind of release to push to GCS. Supported values are kubernetes (default) or federation",
)
pushBuildCmd.PersistentFlags().StringVar(
&pushBuildOpts.releaseType,
"release-kind",
"devel",
"Specify an alternate bucket for pushes (normally 'devel' or 'ci')",
)
pushBuildCmd.PersistentFlags().StringVar(
&pushBuildOpts.versionSuffix,
"version-suffix",
Expand All @@ -147,5 +153,89 @@ func init() {

func runPushBuild(opts *pushBuildOptions) error {
log.Println("unimplemented")

var latest string
releaseKind := opts.releaseKind

// Check if latest build uses bazel
dir, err := os.Getwd()
if err != nil {
log.Fatal(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we let the errors bubble up instead of exiting via log.Fatal here (and at the other places)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that we should keep executing? I think it depends on the error. This one I think we should exit immediately because we will be unable to reliably push a build if we cannot get the working dir.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is:

  • return an error here instead of log.Fatal(...)
  • this error should bubble up all the way to be either handled or results in an os.Exit(...) or such somewhere centrally.

I would not expect a function that returns an error also to shut down the whole process. If nothing else, that is IMHO not really intuitive and hard stupid to test.

}

isBazel, err := release.BuiltWithBazel(dir, releaseKind)
if err != nil {
log.Fatal(err)
}

if isBazel {
log.Println("Using Bazel build version.")
Copy link
Member

Choose a reason for hiding this comment

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

Its a nitty nit, but we could remove the dot at the end of a log to have a uniform logging layout. :)

version, err := release.ReadBazelVersion(dir)
if err != nil {
log.Fatal(err)
}
latest = version
} else {
log.Println("Using Dockerized build version.")
version, err := release.ReadDockerizedVersion(dir, releaseKind)
if err != nil {
log.Fatal(err)
}
latest = version
}

log.Println("Found build version: " + latest)

valid, err := release.IsValidReleaseBuild(latest)
if err != nil {
log.Fatal(err)
}
if !valid {
log.Fatal("build version is not valid for release")
}

if opts.ci && release.IsDirtyBuild(latest) {
log.Fatal(`Refusing to push dirty build with --ci flag given.\n
CI builds should always be performed from clean commits.`)
}

if opts.versionSuffix != "" {
latest += "-" + opts.versionSuffix
}

gcsDest := opts.releaseType

// TODO: is this how we want to handle gcs dest args?
if opts.ci {
gcsDest = "ci"
}

gcsDest += opts.gcsSuffix

releaseBucket := opts.bucket
if !rootOpts.nomock {
u, err := user.Current()
if err != nil {
log.Fatal(err)
}

releaseBucket += "-" + u.Username
}

client, err := storage.NewClient(context.Background())
if err != nil {
log.Fatalf("error fetching gcloud credentials %s... try running \"gcloud auth application-default login\"", err)
}

bucket := client.Bucket(releaseBucket)
if bucket == nil {
log.Fatalf("unable to identify specified bucket for artifacts: %s", releaseBucket)
}

// Check if bucket exists.
if _, err = bucket.Attrs(context.Background()); err != nil {
log.Fatal(err)
}

return nil
}
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module k8s.io/release
go 1.13

require (
cloud.google.com/go v0.38.0 // indirect
cloud.google.com/go v0.38.0
github.com/blang/semver v3.5.1+incompatible
github.com/go-kit/kit v0.9.0
github.com/google/go-github/v28 v28.1.1
Expand Down
23 changes: 23 additions & 0 deletions pkg/release/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "go_default_library",
srcs = ["release.go"],
importpath = "k8s.io/release/pkg/release",
visibility = ["//visibility:public"],
deps = ["//pkg/util:go_default_library"],
)

filegroup(
name = "package-srcs",
srcs = glob(["**"]),
tags = ["automanaged"],
visibility = ["//visibility:private"],
)

filegroup(
name = "all-srcs",
srcs = [":package-srcs"],
tags = ["automanaged"],
visibility = ["//visibility:public"],
)
66 changes: 66 additions & 0 deletions pkg/release/release.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
Copyright 2019 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package release

import (
"io/ioutil"
"regexp"
"strings"

"k8s.io/release/pkg/util"
)

const (
versionReleaseRE = `v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(-[a-zA-Z0-9]+)*\.*(0|[1-9][0-9]*)?`
versionDotZeroRE = `v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.0$`
versionBuildRE = `([0-9]{1,})\+([0-9a-f]{5,40})`
versionDirtyRE = `(-dirty)`
dockerBuildPath = "/_output/release-tars/"
bazelBuildPath = "/bazel-bin/build/release-tars/"
bazelVersionPath = "/bazel-genfiles/version"
dockerVersionPath = "/version"
tarballExtension = ".tar.gz"
)

// BuiltWithBazel determines whether the most recent release was built with Bazel.
func BuiltWithBazel(path string, releaseKind string) (bool, error) {
bazelBuild := path + bazelBuildPath + releaseKind + tarballExtension
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using filepath.Join for concatenating paths.

dockerBuild := path + dockerBuildPath + releaseKind + tarballExtension
return util.MoreRecent(bazelBuild, dockerBuild)
}

// ReadBazelVersion reads the version from a Bazel build.
func ReadBazelVersion(path string) (string, error) {
version, err := ioutil.ReadFile(path + bazelVersionPath)
return string(version), err
}

// ReadDockerizedVersion reads the version from a Dockerized
func ReadDockerizedVersion(path, releaseKind string) (string, error) {
dockerTarball := path + dockerBuildPath + releaseKind + tarballExtension
return util.UntarAndRead(dockerTarball, releaseKind+dockerVersionPath)
}

// IsValidReleaseBuild checks if build version is valid for release.
func IsValidReleaseBuild(build string) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest in returning either a bool or an error here to make its usage simpler.

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 was initially thinking that as well, but I didn't know if there would be a use case for this function in the future where it might be valid to know if there was not a match or if the comparison failed altogether. If we don't think this is necessary, I think a good fix would be to use regexp.MustCompile in init() then just returning MatchString() here!

Copy link
Member

Choose a reason for hiding this comment

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

Hm I see your point, so I guess we can also keep the current approach. :)

return regexp.MatchString("("+versionReleaseRE+`(\.`+versionBuildRE+")?"+versionDirtyRE+"?)", build)
}

// IsDirtyBuild checks if build version is dirty.
func IsDirtyBuild(build string) bool {
return strings.Contains(build, "dirty")
}
50 changes: 49 additions & 1 deletion pkg/util/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,17 @@ limitations under the License.
package util

import (
"archive/tar"
"bufio"
"compress/gzip"
"errors"
"fmt"
"io"
"io/ioutil"
"log"
"os"
"path/filepath"
"strings"
)

/*
Expand Down Expand Up @@ -104,7 +108,7 @@ func FakeGOPATH(srcDir string) (string, error) {
log.Printf("GOPATH: %s", os.Getenv("GOPATH"))

gitRoot := fmt.Sprintf("%s/src/k8s.io", baseDir)
if err := os.MkdirAll(gitRoot, 0o755); err != nil {
if err := os.MkdirAll(gitRoot, os.FileMode(int(0755))); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, what is the benefit of doing it like this? 🙃

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 was getting a compiler error on my local, but I think my config was just wrong. I'll change back :)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, maybe you have no go 1.13 on your local machine?

return "", err
}
gitRoot = filepath.Join(gitRoot, "kubernetes")
Expand All @@ -122,3 +126,47 @@ func FakeGOPATH(srcDir string) (string, error) {

return gitRoot, nil
}

// UntarAndRead opens a tarball and reads contents of a file inside.
func UntarAndRead(tarPath, filePath string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we wanna write some tests for this function and MoreRecent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! In my check list above

Copy link
Contributor

Choose a reason for hiding this comment

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

nits:

  • Do we maybe want to return the io.Reader instead of a string? That way it
    • would be more ideomatic
    • a caller could wrap a LimitedReader around it or do whatever
  • Is a name like ReadFileFromGzippedTar(...) a "truer" name. Would it be a better name? Not really ... 🤔#namingThings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree to both of these points :) This structure was mostly a result of trying to mimic the exact behavior of

LATEST=$(tar -O -xzf $KUBE_ROOT/_output/release-tars/$FLAGS_release_kind.tar.gz $FLAGS_release_kind/version)

file, err := os.Open(tarPath)
if err != nil {
return "", err
}

archive, err := gzip.NewReader(file)
if err != nil {
return "", err
}
tr := tar.NewReader(archive)

for {
h, err := tr.Next()
if err == io.EOF {
break // End of archive
}

if h.Name == filePath {
file, err := ioutil.ReadAll(tr)
return strings.TrimSuffix(string(file), "\n"), err
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this (trimming of a newline) a general thing we need to do when extracting a file from a tarball, or is this specific to our case (reading that version file)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more of a specific use-case thing, which, if this function is made more general as suggested above, should probably be deferred to the user of the function (i.e. the push build cmd). Thanks for pointing this out!

}
}

return "", errors.New("unable to find file in tarball")
}

// MoreRecent determines if file at path a was modified more recently than file at path b.
func MoreRecent(a, b string) (bool, error) {
// TODO: default to existing file if only one exists?
fileA, err := os.Stat(a)
if err != nil {
return false, err
}

fileB, err := os.Stat(b)
if err != nil {
return false, err
}

return (fileA.ModTime().Unix() > fileB.ModTime().Unix()), nil
}