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

testing: initial code for release helper tool #2114

Merged
merged 10 commits into from
May 28, 2019

Conversation

eliben
Copy link
Member

@eliben eliben commented May 15, 2019

Initial release helper tool for #2111. Doesn't help with subsequent releases yet - will add that functionality as we go.

@googlebot googlebot added the cla: yes Google CLA has been signed! label May 15, 2019
internal/testing/releasehelper.go Outdated Show resolved Hide resolved
internal/testing/releasehelper.go Show resolved Hide resolved
internal/testing/releasehelper.go Outdated Show resolved Hide resolved
internal/testing/releasehelper.go Show resolved Hide resolved
@eliben eliben marked this pull request as ready for review May 24, 2019 17:27
@eliben eliben requested a review from vangent May 24, 2019 17:28
@eliben
Copy link
Member Author

eliben commented May 24, 2019

There's enough functionality now to help with the initial split release. Will add more flags and goodies later as we go along and learn the ropes of juggling multiple modules

@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

Merging #2114 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2114      +/-   ##
==========================================
- Coverage   70.59%   70.53%   -0.06%     
==========================================
  Files         104      104              
  Lines       12508    12508              
==========================================
- Hits         8830     8823       -7     
- Misses       3019     3031      +12     
+ Partials      659      654       -5
Impacted Files Coverage Δ
internal/docstore/driver/codec.go 91.22% <0%> (-3.58%) ⬇️
internal/cmd/gocdk/main.go 25.84% <0%> (-2.25%) ⬇️
pubsub/kafkapubsub/kafka.go 83.39% <0%> (-0.8%) ⬇️
pubsub/azuresb/azuresb.go 26.59% <0%> (-0.75%) ⬇️
pubsub/mempubsub/mem.go 82.31% <0%> (-0.12%) ⬇️
pubsub/gcppubsub/gcppubsub.go 84.04% <0%> (-0.1%) ⬇️
pubsub/rabbitpubsub/rabbit.go 81.43% <0%> (-0.07%) ⬇️
pubsub/awssnssqs/awssnssqs.go 84.26% <0%> (-0.06%) ⬇️
internal/docstore/driver/document.go 71.08% <0%> (ø) ⬆️
internal/cmd/gocdk/serve.go 23.21% <0%> (ø) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94ad9f8...dd2932b. Read the comment docs.

internal/testing/releasehelper.go Outdated Show resolved Hide resolved
internal/testing/releasehelper.go Outdated Show resolved Hide resolved
if r.Path == "gocloud.dev" {
reqPath = "."
} else {
reqPath = r.Path[12:]
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, something like this:

if reqPath := strings.TrimPrefix(r.Path, "gocloud.dev"); reqPath != r.Path {
  if reqPath == "" {
    reqPath = "."
  }
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. Mine feels a bit simpler, so I'll stick with it if you don't mind

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I don't really find the 12: that readable, maybe TrimPrefix there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done

internal/testing/releasehelper.go Show resolved Hide resolved
internal/testing/releasehelper.go Show resolved Hide resolved
internal/testing/releasehelper.go Outdated Show resolved Hide resolved
internal/testing/releasehelper.go Show resolved Hide resolved
if r.Path == "gocloud.dev" {
reqPath = "."
} else {
reqPath = r.Path[12:]
Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I don't really find the 12: that readable, maybe TrimPrefix there?

internal/testing/releasehelper.go Show resolved Hide resolved
"strings"
)

// TODO(eliben): add bumpversion flag that bumps version by 1, or to a version
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling that when we add these it will be better to use commands rather than flags. Either commands that are high level like prep which package up a bunch of lower-level things, or low-level commands that you do one at a time (releasehelper bumpversion, releasehelper addreplace). Otherwise it may be tricky to define the semantics for various combinations of flags (e.g., these two existing flags already depend on each other in some way; it doesn't make sense to set both or neither. With more flags, that may get more complex).

But, we can revisit later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I see what you're saying, been thinking about this. It's hard to predict what future functionailty we'll want though and I don't want to over-YAGNI it too much right now. We'll enhance it as we go and learn the ropes of the new process

@vangent vangent changed the title testing: strawman code for release helper tool testing: initial code for release helper tool May 28, 2019
@eliben eliben merged commit 00b1a53 into google:master May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA has been signed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants