Patch release announce in golang #1040
Conversation
|
@hoegaarden: Adding label: Reasons for blocking this PR:[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.] 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. |
k8s-ci-robot
added
do-not-merge/blocked-paths
| } | ||
|
|
||
| var err error | ||
| if opts.Nomock, err = cmd.Flags().GetBool("nomock"); err != nil { |
There was a problem hiding this comment.
nomock is already part of the rootOptions structure, so there is no need to add it twice.
There was a problem hiding this comment.
- This is not "adding" the flag, this is asking the parent command to for a flag's value. Any parent command could have been responsible for adding/parsing that flag, this is a way to retrieve it's value from within a child.
- I don't want to rely on any shared state between the parent (in our case, the root command) and this command.
cobragives me options to communicate with / query the parent command without this "external" shared state. - This decouples this command from any other / the root command. The only touch point with any other command is when we register it. Potentially we could move this command, change the parent, ... without really bothering this command.
| if opts.Nomock, err = cmd.Flags().GetBool("nomock"); err != nil { | ||
| return err | ||
| } | ||
| if opts.K8sRepoPath, err = cmd.Flags().GetString("repo"); err != nil { |
There was a problem hiding this comment.
repoPath is already part of the rootOptions structure, so there is no need to add it twice.
There was a problem hiding this comment.
see #1040 (comment)
| --output=- | ||
| ` | ||
|
|
||
| func (r *Formatter) MarkdownToHTML(markdown, title string) (string, error) { |
There was a problem hiding this comment.
This can be done with blackfriday.Run([]byte(markdown)) and a HTML template. Is this already on your follow-up list?
There was a problem hiding this comment.
Sweet!
Yes, that is one of the things. From the top of my head:
- change the release noter
- change the formatter
- change the workspace / version querying
Non of those things should have external dependencies, shell out, ...
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
Let's iterate on this and bring it in as soon as possible. Unfortunately the linter still complains. |
Yeah ... lookig into that. No idea why though. Will ping you once that's figured out. |
| # TODO: move to the kubernetes-release-test | ||
| - kmsKeyName: projects/cf-london-servces-k8s/locations/global/keyRings/hhorl-k8s-release/cryptoKeys/sendgrid | ||
| secretEnv: | ||
| SENDGRID_API_KEY: CiQAkn5y0WUR0kZIqj1v4UW2McFdlceel2zptSa468+Ir/gDeh0SbwDMTYRJdFtqt2x2hGPN7oOoMs2lCLFoY+oh4Mb1b/nQehooMYQuNwupxOpnwE+d/GbEFhwtMg9wbA5zhOMYPe0OLsSAXvt4w/IC1Z701Ev3CMdT6ZjicillZoBlFYhVlFPQLWcm8jtobdQcDj91eg== |
There was a problem hiding this comment.
@justaugustus how would I go about that? If I understand #1031 (comment) correctly you opt against adding new encrypted secrets to GCP (for now).
#1045 should fix the linting issues |
We encrypt the secrets GITHUB_TOKEN & SENDGRID_API_KEY with the cloud KMS and use those in GCB. This means, that those envs won't get logged in the GCB console.
By moving the log setup from `PreRunE` to `PersistentPreRunE` the log setup also run for and therefore configures logging for sub commands.
k8s-ci-robot
removed
the
lgtm
|
/test pull-release-cluster-up |
|
|
||
| i := strings.IndexRune(gitVersion, '-') | ||
| if i < 0 { | ||
| return "", fmt.Errorf("cannot extract upcoming version from gitVersion %q", gitVersion) |
There was a problem hiding this comment.
can be made consistent with the above:
fmt.Errorf("workspace status has no field 'gitVersion': %q", status)
There was a problem hiding this comment.
I think these are two distinct error cases:
- we cannot even find the thing we are looking for, ie.
gitVersion - the thing we are looking for is there, but it is not in the shape or form we expected it to be, ie. currently in this case that means it does not include a
-
There was a problem hiding this comment.
one seems to have a quoted gitVersion
There was a problem hiding this comment.
oh ... got it! Will do this in a follow up!
pkg/patch/internal/exec.go
Outdated
| return s | ||
| } | ||
| partLen := (maxLen - 3) / 2 | ||
| return s[:partLen] + " … " + s[len(s)-partLen:] |
There was a problem hiding this comment.
should be ... (3x .) instead of the "Unicode Character 'HORIZONTAL ELLIPSIS' (U+2026)".
There was a problem hiding this comment.
Happy to do make change!
I just want to understand why you are saying
should be ... (3x .) instead of the "Unicode Character [...]
Are there some guidelines about not using unicode in errors/error messages or such?
There was a problem hiding this comment.
there could be a guide somewhere, who knows.
for English it makes a lot of sense to stick to ASCII.
someone attempted to add a Unicode checker for k/k because we were getting a lot stray Unicode characters like this one, but the proposal failed, AFAIK.
| } | ||
|
|
||
| output, err := f.MarkdownToHTML(tc.content, tc.title) | ||
| it.CheckErrSub(t, err, tc.expectedErr) |
There was a problem hiding this comment.
IMO, instead of matching error strings, the expectedErr can be a bool.
There was a problem hiding this comment.
The tests do in fact check on the error message itself, ie. they kind of test where the error came from.
| result2 error | ||
| } | ||
| invocations map[string][][]interface{} | ||
| invocationsMutex sync.RWMutex |
There was a problem hiding this comment.
are the two mutexes here to conform unit tests?
can they be combined into a single RW mutex if this is for unit tests only?
i may have missed related concurrency in the business logic.
IMO it should be avoided to prevent confusion and debugging difficulties.
There was a problem hiding this comment.
This is generated code. I guess most of your concerns about the fakes stem from the fact that counterfeiter is written in a generic way, is able to create fakes for functions & interfaces and does not make assumptions if the thing it fakes is used in concurrent code or not.
Things in .../<something>fakes/... or which are named fake_<something>.go are most probably counterfeiter generated fakes, are tagged as being autogenerated (e.g. here) and thus can be ignored IMHO.
Having said that, if you have concerns with the counterfeiter fakes, let me know and I can see to address them upstream.
There was a problem hiding this comment.
looks like i missed the // Code generated by counterfeiter. DO NOT EDIT. part
| result1 error | ||
| } | ||
| invocations map[string][][]interface{} | ||
| invocationsMutex sync.RWMutex |
There was a problem hiding this comment.
alternatively this could have been organized like so:
type someType1 struct {
sync.RWMutex
...
}
type someType2 struct {
sync.RWMutex
...
}
type formatter struct {
someInstance1 someType1
someInstance2 someType2
...
}
There was a problem hiding this comment.
same as #1040 (comment)
| fake.invocations[key] = append(fake.invocations[key], args) | ||
| } | ||
|
|
||
| var _ patch.Formatter = new(FakeFormatter) |
There was a problem hiding this comment.
it's more common to write the type assertions with = &FakeFormatter{}
the compiler may or may not optimize the runtime call here.
There was a problem hiding this comment.
same as #1040 (comment)
| type CommandCreator func(path string, args ...string) Cmd | ||
|
|
||
| func (c CommandCreator) create(path string, args ...string) Cmd { | ||
| if c == nil { |
There was a problem hiding this comment.
can create() be called multiple times on the same object?
is this check needed?
There was a problem hiding this comment.
- technically,
createcan be called multiple times. I don't think here anything calls it multiple times, but I don't see a reason why it could or should not be done. - the
nilcheck is there, because I appreciate thenilreceiver. If users have a thing which is of the interface typeCommandCreator, but have not set it to anything (ie. it isnil), things still work without forcing users to donilchecks. To rephrase that: users can use a thing of typeCommandCreatoranywhere, don't need to initialise it but can just callcreateon it and it will work; they are however also free to overwrite the thing and change the implementation. This allows to just use the zero type of structs which hold a thing of typeCommandCreator. - I put most of the stuff into the
internalpackage to keep the external facing API to a minimum. I could see certain things to be useful for the whole module or even beyond. If we'd decide to move things around we probably also need to export certain things, e.g. thecreatemethod here specifically.
There was a problem hiding this comment.
i'd prefer this to panic on user calls with nil objects, but this pattern is also present in k/k.
i complained to the people there too. :old-man-yells-at-cloud:
There was a problem hiding this comment.
thanks for working on this @hoegaarden
i've added some non-blocking nits in this "pass-by" review.
Some parts (release notes, workspace, pandoc) currently still shell out. We can change that gradually by change the implementations of the respective structs.
Remove the bash implementation, move to the golang based tool. The tool currently still shells out and has some external dependencies. Therefore we, for now, just `go run` it in the `k8s-cloud-builder` images, where we have all the tools available.
This is deliberatly NOT a krel subcommand, as this is just a small wrapper around `gcloud` for submitting the a job running `krel patch-announce ...`.
1. It was a no-op anyway 1. It was called for each sub-command, called by the package's `init` function If we want to bring that back, we should do it deliberately.
k8s-ci-robot
removed
the
lgtm
|
@neolit123, thanks a ton for your review! I have addressed most of your comments & have questions regarding some of them. /PTAL |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@hoegaarden -- Let's get this in and iterate! The next thing I'd like to see is how we can merge the streams between this and krel announce --type {prerelease,release,...}where /lgtm |
k8s-ci-robot
removed
the
do-not-merge/hold
justaugustus
removed
the
do-not-merge/blocked-paths
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, hasheddan, hoegaarden, justaugustus, neolit123, saschagrunert 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 |
|
(p.s. I'll fix the Sendgrid key in a follow-up) |
This is currently still shelling out to the release notes and other tools.
Switching that over to compiled in alternatives are follow-ups.
This currently is using sendgrid with my token to send the mail. If we want to use that, we should create a sendgrid account for SIG-release or move to a different service.
Most of the changes is actually generated code for the test fakes.
There are quite some TODOs in there, I'll try to address those individually as follow up PRs once this general approach is accepted.
/cc @saschagrunert @justaugustus @tpepper
public/exported API for the k8s.io/release/patch package