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

keep only master branch for latest #160

Merged
merged 12 commits into from Nov 9, 2017

Conversation

Projects
None yet
4 participants
@appleboy
Copy link
Member

appleboy commented Nov 2, 2017

comment from go-gitea/gitea#2827 (comment)

cc @tboerger

Signed-off-by: Bo-Yi Wu appleboy.tw@gmail.com

keep only master branch for latest
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Nov 2, 2017

I mentioned this briefly in the announcement

Planned improvements include automated tags for pushes to non-master branches.

The default behavior I'm planning to implement is that master is always set to latest and non-master branches use the branch name, converted to a slug, as the tag. So for example

  • master would be set to latest
  • develop would be set to develop
  • feature/foo-bar would be set to feature-foo-bar

The defaults are meant to be very opinionated, so at this time I would prefer not to add any additional yaml attributes and keep the logic as automated as possible.

@lafriks

This comment has been minimized.

Copy link

lafriks commented Nov 2, 2017

@bradrydzewski default branch could still be something other from master so it would still be good to allow to set what branch is going to be latest tag

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Nov 2, 2017

The default branch could passed as a DRONE_ environment variable as an alternative to a yaml configuration parameter.

appleboy added some commits Nov 3, 2017

add DRONE_ environment variable
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
remove additional yaml attributes
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Nov 3, 2017

@bradrydzewski I remove the additional yaml attributes and passed as a DRONE_REPO_BRANCH environment variable as an alternative to a yaml configuration parameter.

},
cli.StringFlag{
Name: "default.branch",
Usage: "defualt repository branch",

This comment has been minimized.

@tboerger
tags.go Outdated
func DefaultTags(ref, commitBranch, defaultBranch string) []string {

if defaultBranch != "" &&
commitBranch != defaultBranch &&

This comment has been minimized.

@tboerger

tboerger Nov 6, 2017

Member

Please don't wrap that across multiple lines

update
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Nov 6, 2017

@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Nov 7, 2017

@bradrydzewski any suggestion on this?

EnvVar: "DRONE_COMMIT_BRANCH",
},
cli.StringFlag{
Name: "default.branch",

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 7, 2017

Member

let's call this repo.branch which is consistent with some of the other plugins and the plugin starter

This comment has been minimized.

@appleboy
tags.go Outdated
@@ -26,10 +26,16 @@ func DefaultTagSuffix(ref, suffix string) []string {

// DefaultTags returns a set of default suggested tags based on
// the commit ref.
func DefaultTags(ref string) []string {
func DefaultTags(ref, commitBranch, defaultBranch string) []string {

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 7, 2017

Member

I think we can remove commitBranch since this information is already passed in the ref field. For example if commitBranch == master we know that ref == refs/heads/master

This comment has been minimized.

@appleboy
tags.go Outdated
func DefaultTags(ref, commitBranch, defaultBranch string) []string {

if defaultBranch != "" && commitBranch != defaultBranch && !strings.HasPrefix(ref, "refs/tags/") {
return []string{}

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 7, 2017

Member

I am not sure we ever want to return an empty string here. We should always return a default tag.

This comment has been minimized.

@appleboy

appleboy Nov 9, 2017

Member

maybe we can add a new flag to confirm that if the user wants to build tags from another branch which is not equal the default branch.

  1. default branch equal to commit branch. return latest tag
  2. new flag BuildFromBranch
    2.1: default branch not equal to commit branch. return commit branch tag if BuildFromBranch is true
    2.2: default branch not equal to commit branch. return empty tag if BuildFromBranch is false

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 9, 2017

Member

default branch not equal to commit branch. return empty tag

An empty tag is not valid and will fail to publish. What is the goal with returning an empty branch?

This comment has been minimized.

@appleboy

appleboy Nov 9, 2017

Member

@bradrydzewski I meant empty value like []string{}

This comment has been minimized.

@appleboy

appleboy Nov 9, 2017

Member

and we can check the tag array in Exec like

	if len(p.Build.Tags) == 0 {
		fmt.Println("build tags not found")
		return nil
	}

This comment has been minimized.

@appleboy

appleboy Nov 9, 2017

Member

@bradrydzewski Any other suggestion?

replace commit branch with refs
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
tags.go Outdated
@@ -9,8 +9,8 @@ import (

// DefaultTagSuffix returns a set of default suggested tags
// based on the commit ref with an attached suffix.
func DefaultTagSuffix(ref, suffix string) []string {
tags := DefaultTags(ref)
func DefaultTagSuffix(ref, suffix, defaultBranch string) []string {

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 9, 2017

Member

lets reverse the ordering since string functions usually put the prefix/ suffix as the last variable in the function.

- ref, suffix, defaultBranch string
+ ref, defaultBranch, suffix string

This comment has been minimized.

@appleboy
tags.go Outdated
func DefaultTags(ref, defaultBranch string) []string {

if defaultBranch != "" && strings.HasPrefix(ref, "refs/heads/") {
if stripHeadPrefix(ref) != defaultBranch {

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 9, 2017

Member

I think we should just use if strings.TrimPrefix(ref, "refs/heads") != defaultBranch. I'm not sure we need a helper function since this is not used anywhere else and it is a 1-liner.

This comment has been minimized.

@appleboy

appleboy Nov 9, 2017

Member

We need to make sure the ref is branch or tag so it is not working for using strings.TrimPrefix(ref, "refs/heads") != defaultBranch

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 9, 2017

Member

if defaultBranch != "" should not be required because defaultBranch will never be empty

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 9, 2017

Member

but really I think this whole block should be removed, since we should never return an empty tag. If there is a goal (not publishing images for branches) we can discuss this, but returning an empty tag is not the way I would like to enforce that logic.

reverse the orderin
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
docker.go Outdated
Repo string // Docker build repository
LabelSchema []string // Label schema map
Branch string // Docker build branch
DefaultBranch string // Docker latest branch

This comment has been minimized.

@bradrydzewski

bradrydzewski Nov 9, 2017

Member

I think this can be removed since it isn't being used

This comment has been minimized.

@appleboy
@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Nov 9, 2017

I think we need to take a step back and define the requirements. I am having trouble understanding the purpose of returning empty tags (e.g. what is the use case this is trying to solve).

remove commit branch frone struct.
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Nov 9, 2017

@bradrydzewski

We create a new branch for release process in Gitea but we don't want to build latest tag in another branch. So we need to keep only master branch for latest tag.

ref: go-gitea/gitea#2827

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Nov 9, 2017

so maybe we change the logic to skip and exit if the build is not a tag and not the default branch?

if c.Bool("tags.auto") {
  if docker.UseDefaultTag( // return true if not default branch, or not tag
    c.String("commit.ref"),
    c.String("repo.branch"),
  ) {
    plugin.Build.Tags = docker.DefaultTagSuffix(
      c.String("commit.ref"),
      c.String("tags.suffix"),
    )
  } else {
    log.Printf("skipping automated docker build for %s", ref)
    return
  }
}

and in this case DefaultTagSuffix would not need to be modified

@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Nov 9, 2017

@bradrydzewski OK. I will update later.

UseDefaultTag for skip build if not default branch.
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>

@appleboy appleboy force-pushed the appleboy:defalut branch from 75d7a54 to 5fa26eb Nov 9, 2017

appleboy added some commits Nov 9, 2017

remove DefaultBranch
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
add unit test.
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>

appleboy added some commits Nov 9, 2017

remove default branch
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
update comment
Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Nov 9, 2017

@bradrydzewski Done

The default branch is master.

  • refs/heads/master => build for latest tag
  • refs/heads/develop => skip
  • refs/heads/release => skip
  • refs/tags/v1.0.0 => build
@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Nov 9, 2017

cool, thanks for being patient with me :)

appreciate the pull request

@bradrydzewski bradrydzewski merged commit 55fd78d into drone-plugins:master Nov 9, 2017

1 check passed

continuous-integration/drone/pr the build was successful
Details

@appleboy appleboy deleted the appleboy:defalut branch Nov 10, 2017

@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Nov 10, 2017

@bradrydzewski Need to review this PR cncd/pipeline#27 just add default branch of repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment