Rename master to aggregator #847
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
johnSchnake
commented
Aug 19, 2019
| type aggregatorInput struct { | ||
| noExit bool | ||
| kubecfg Kubeconfig | ||
| } |
There was a problem hiding this comment.
Just a single type/variable instead of 2 globals. I then create a single instance of this variable below in NewCmdAggregator which returns a closure around that instance.
johnSchnake
commented
Aug 19, 2019
| Use: "aggregator", | ||
| Short: "Runs the aggregator component (for internal use)", | ||
| Long: "Sonobuoy is an introspective kubernetes component that generates reports on cluster conformance, configuration, and more", | ||
| Run: func(cmd *cobra.Command, args []string) { |
There was a problem hiding this comment.
Didn't want to change this to RunE but will do that with a bunch of commands at the same time eventually. We should be writing our code in a way that allows us to bubble up errors to the main Execute method so we can centralize error handling rather than having logging/os.Exit all over the place.
johnSchnake
commented
Aug 19, 2019
| @@ -65,10 +66,6 @@ var singleNodeCmd = &cobra.Command{ | |||
| Args: cobra.ExactArgs(0), | |||
| } | |||
|
|
|||
| func runGatherHelp(cmd *cobra.Command, args []string) { | |||
| cmd.Help() | |||
| } | |||
There was a problem hiding this comment.
No need to add this, if you don't set a Run or RunE it will show usage messages already.
johnSchnake
commented
Aug 19, 2019
johnSchnake
commented
Aug 19, 2019
johnSchnake
commented
Aug 19, 2019
| @@ -107,7 +107,7 @@ func (p *Plugin) createDaemonSetDefinition(hostname string, cert *tls.Certificat | |||
| Labels: labels, | |||
| Annotations: annotations, | |||
| OwnerReferences: []metav1.OwnerReference{ | |||
| metav1.OwnerReference{ | |||
There was a problem hiding this comment.
Just a gofmt change; I have my editor set to use gofmt -s which simplifies more than the default/minimum.
johnSchnake
commented
Aug 19, 2019
| @@ -116,7 +116,7 @@ type AggregationConfig struct { | |||
|
|
|||
| // WorkerConfig is the file given to the sonobuoy worker to configure it to phone home. | |||
| type WorkerConfig struct { | |||
| // MasterURL is the URL we talk to for submitting results | |||
| // MasterURL is the URL we talk to the aggregator pod on for submitting results | |||
There was a problem hiding this comment.
Did not change this since it is part of an exported struct/config.
johnSchnake
commented
Aug 19, 2019
| @@ -17,7 +17,7 @@ | |||
|
|
|||
| ## How it works now | |||
|
|
|||
| Sonobuoy uses a worker-master model, where a master delegates tasks to worker | |||
There was a problem hiding this comment.
This one line from some old docs had gotten updated; I don't really care about the older ones. If its no different to anyone else I'll just leave it rather than trying to fix up other sections in this (or other older docs)
There was a problem hiding this comment.
Are these files even linked to from the site? I can't find any references to them. :-/
There was a problem hiding this comment.
The enhancements aren't linked but they do actually exist if you manually write the URL.
They were really for internal discussion/documentation back then; I made the decision that I didn't really want/need them to be displayed in our online docs site though.
Codecov Report
@@ Coverage Diff @@
## master #847 +/- ##
=========================================
- Coverage 46.67% 46.48% -0.2%
=========================================
Files 75 75
Lines 4831 4830 -1
=========================================
- Hits 2255 2245 -10
- Misses 2448 2454 +6
- Partials 128 131 +3
Continue to review full report at Codecov.
|
zubron
suggested changes
Aug 23, 2019
cmd/sonobuoy/app/master.go
Outdated
| logrus.Info("no-exit was specified, sonobuoy is now blocking") | ||
| select {} | ||
| } | ||
|
|
||
| os.Exit(errcount) | ||
| return fmt.Errorf("%v errors encountered during execution", errcount) |
There was a problem hiding this comment.
By using fmt.Errorf here, we're always returning a non-nil error. We should be returning nil in the case where errcount is 0.
| @@ -17,7 +17,7 @@ | |||
|
|
|||
| ## How it works now | |||
|
|
|||
| Sonobuoy uses a worker-master model, where a master delegates tasks to worker | |||
There was a problem hiding this comment.
Are these files even linked to from the site? I can't find any references to them. :-/
Slightly more intuitive and useful name. Kept the old name as an alias which we can decide to remove later if we want. Did not update a few of the config values or script names that are named with 'master' in order to avoid potentially breaking changes. Fixes #238 Signed-off-by: John Schnake <[email protected]>
zubron
approved these changes
Aug 23, 2019
zubron
added a commit
that referenced
this pull request
Jul 2, 2020
We renamed the `master` command to `aggregator` in #847 (released in v0.15.3) but left the previous command there as an alias. Given that we have had 3 minor releases since this and have phased out all use, we can remove this alias. Also rename the file to match the new command name. Signed-off-by: Bridget McErlean <[email protected]>
zubron
added a commit
to zubron/sonobuoy
that referenced
this pull request
Jul 7, 2020
We renamed the `master` command to `aggregator` in vmware-tanzu#847 (released in v0.15.3). However, `master` was still being used in some of our Kubernetes resource labels and names. We want to remove uses of insensitive language and as such use different labels for identifying resources. Now, any aggregator resources will have the label `sonobuoy-component: aggregator` and plugin resources will have `sonobuoy-component: plugin`. The existing label is still in place as it was used to identify the aggregator pod (which is required most sonobuoy commands once a run has started). Removing it now would prevent users with older clients from using newer images, or querying the state of runs started by newer clients. Instead, both labels will be applied howeve the `sonobuoy-component: aggregator` label is the preferred label. If the aggregator pod cannot be found using the new label, the client will retry using the now deprecated label. When retrying, the client will display a warning that it is attempting to find the pod using the deprecated label. Newer clients will be able to query the state of runs with old labels applied, and older clients will be able to query the state of runs created by the newer clients. The name of the service created by Sonobuoy has been renamed to `sonobuoy-aggregator`. This has no user facing impact. Signed-off-by: Bridget McErlean <[email protected]>
zubron
added a commit
to zubron/sonobuoy
that referenced
this pull request
Jul 7, 2020
We renamed the `master` command to `aggregator` in vmware-tanzu#847 (released in v0.15.3). However, `master` was still being used in some of our Kubernetes resource labels and names. We want to remove uses of insensitive language and as such use different labels for identifying resources. Now, any aggregator resources will have the label `sonobuoy-component: aggregator` and plugin resources will have `sonobuoy-component: plugin`. The existing label is still in place as it was used to identify the aggregator pod (which is required most sonobuoy commands once a run has started). Removing it now would prevent users with older clients from using newer images, or querying the state of runs started by newer clients. Instead, both labels will be applied howeve the `sonobuoy-component: aggregator` label is the preferred label. If the aggregator pod cannot be found using the new label, the client will retry using the now deprecated label. When retrying, the client will display a warning that it is attempting to find the pod using the deprecated label. Newer clients will be able to query the state of runs with old labels applied, and older clients will be able to query the state of runs created by the newer clients. The name of the service created by Sonobuoy has been renamed to `sonobuoy-aggregator`. This has no user facing impact. Signed-off-by: Bridget McErlean <[email protected]>
zubron
added a commit
that referenced
this pull request
Jul 7, 2020
We renamed the `master` command to `aggregator` in #847 (released in v0.15.3). However, `master` was still being used in some of our Kubernetes resource labels and names. We want to remove usage of insensitive language and as such use different labels for identifying resources. Now, any aggregator resources will have the label `sonobuoy-component: aggregator` and plugin resources will have `sonobuoy-component: plugin`. The existing label is still in place as it was used to identify the aggregator pod (which is required most sonobuoy commands once a run has started). Removing it now would prevent users with older clients from using newer images, or querying the state of runs started by newer clients. Instead, both labels will be applied howeve the `sonobuoy-component: aggregator` label is the preferred label. If the aggregator pod cannot be found using the new label, the client will retry using the now deprecated label. When retrying, the client will display a warning that it is attempting to find the pod using the deprecated label. Newer clients will be able to query the state of runs with old labels applied, and older clients will be able to query the state of runs created by the newer clients. The name of the service created by Sonobuoy has been renamed to `sonobuoy-aggregator`. This has no user facing impact. Signed-off-by: Bridget McErlean <[email protected]>
zubron
added a commit
to zubron/sonobuoy
that referenced
this pull request
Jul 8, 2020
We renamed the `master` command to `aggregator` in vmware-tanzu#847 (released in v0.15.3). However, we left some of the existing uses in place to prevent backwards incompatibility. This change renames the `MasterURL` field in `WorkerConfig` to `AggregatorURL` in keeping with our terminology elsewhere. Worker containers are configured using environment variables. This change renames the environment variable but also adds some compatibility for the original environment variable to still be processed. It is possible for different image versions to be used for the aggregator and the worker (although it would have to be explicitly set in the generated manifest). This changes allows older versions of Sonobuoy to be used as the aggregator image but does not allow older versions to be used as the worker image. Although some compatibility is broken, this seems acceptable as we don't recommend mixing versions of images within a single sonobuoy run. Signed-off-by: Bridget McErlean <[email protected]>
zubron
added a commit
to zubron/sonobuoy
that referenced
this pull request
Jul 8, 2020
We renamed the `master` command to `aggregator` in vmware-tanzu#847 (released in v0.15.3). However, we left some of the existing uses in place to prevent backwards incompatibility. This change renames the `MasterURL` field in `WorkerConfig` to `AggregatorURL` in keeping with our terminology elsewhere. Worker containers are configured using environment variables. This change renames the environment variable but also adds some compatibility for the original environment variable to still be processed. It is possible for different image versions to be used for the aggregator and the worker (although it would have to be explicitly set in the generated manifest). This changes allows older versions of Sonobuoy to be used as the aggregator image but does not allow older versions to be used as the worker image. Although some compatibility is broken, this seems acceptable as we don't recommend mixing versions of images within a single sonobuoy run. Signed-off-by: Bridget McErlean <[email protected]>
zubron
added a commit
to zubron/sonobuoy
that referenced
this pull request
Jul 8, 2020
We renamed the `master` command to `aggregator` in vmware-tanzu#847 (released in v0.15.3). However, we left some of the existing uses in place to prevent backwards incompatibility. This change renames the `MasterURL` field in `WorkerConfig` to `AggregatorURL` in keeping with our terminology elsewhere. Worker containers are configured using environment variables. This change renames the environment variable but also adds some compatibility for the original environment variable to still be processed. It is possible for different image versions to be used for the aggregator and the worker (although it would have to be explicitly set in the generated manifest). This changes allows older versions of Sonobuoy to be used as the aggregator image but does not allow older versions to be used as the worker image. Although some compatibility is broken, this seems acceptable as we don't recommend mixing versions of images within a single sonobuoy run. Signed-off-by: Bridget McErlean <[email protected]>
zubron
added a commit
to zubron/sonobuoy
that referenced
this pull request
Jul 8, 2020
We renamed the `master` command to `aggregator` in vmware-tanzu#847 (released in v0.15.3). However, we left some of the existing uses in place to prevent backwards incompatibility. This change renames the `MasterURL` field in `WorkerConfig` to `AggregatorURL` in keeping with our terminology elsewhere. Worker containers are configured using environment variables. This change renames the environment variable but also adds some compatibility for the original environment variable to still be processed. It is possible for different image versions to be used for the aggregator and the worker (although it would have to be explicitly set in the generated manifest). This changes allows older versions of Sonobuoy to be used as the aggregator image but does not allow older versions to be used as the worker image. Although some compatibility is broken, this seems acceptable as we don't recommend mixing versions of images within a single sonobuoy run. Signed-off-by: Bridget McErlean <[email protected]>
zubron
added a commit
that referenced
this pull request
Jul 8, 2020
We renamed the `master` command to `aggregator` in #847 (released in v0.15.3). However, we left some of the existing uses in place to prevent backwards incompatibility. This change renames the `MasterURL` field in `WorkerConfig` to `AggregatorURL` in keeping with our terminology elsewhere. Worker containers are configured using environment variables. This change renames the environment variable but also adds some compatibility for the original environment variable to still be processed. It is possible for different image versions to be used for the aggregator and the worker (although it would have to be explicitly set in the generated manifest). This changes allows older versions of Sonobuoy to be used as the aggregator image but does not allow older versions to be used as the worker image. Although some compatibility is broken, this seems acceptable as we don't recommend mixing versions of images within a single sonobuoy run. Signed-off-by: Bridget McErlean <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Slightly more intuitive and useful name. Kept the old name
as an alias which we can decide to remove later if we want.
Did not update a few of the config values or script names
that are named with 'master' in order to avoid potentially
breaking changes.
Which issue(s) this PR fixes
Fixes #238
Special notes for your reviewer:
Adding comments inline but also adding a few things here for clarity:
run_master.shrun_master.shto use the newaggregatormastercommand toaggregatorbut keptmasteras an alias so there should be no problem with old clients/scripts calling newer Sonobuoy images.Release note: