Add version support for all binaries #162
Conversation
k8s-ci-robot
added
approved
|
I would recommend reviewing by commit |
| @@ -44,6 +44,10 @@ func (d *Deployer) Init() error { | |||
|
|
|||
| // Initialize should only be called by init(), behind a sync.Once | |||
| func (d *Deployer) Initialize() error { | |||
| if d.ClusterVersion == "" && d.LegacyClusterVersion != "" { | |||
| klog.Warningf("--version is deprecated please use --cluster-version") | |||
| if err2 != nil { | ||
| return err2 | ||
| } | ||
| if err := meta.Add("kubetest-version", os.Getenv("KUBETEST2_VERSION")); err != nil { |
There was a problem hiding this comment.
this currently tries to match with
kubernetes/test-infra#20886 (comment)
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
| @@ -44,6 +44,10 @@ func (d *Deployer) Init() error { | |||
|
|
|||
| // Initialize should only be called by init(), behind a sync.Once | |||
| func (d *Deployer) Initialize() error { | |||
| if d.ClusterVersion == "" && d.LegacyClusterVersion != "" { | |||
| klog.Warningf("--version is deprecated please use --cluster-version") | |||
| @@ -29,7 +29,8 @@ type UpOptions struct { | |||
| NumNodes int `flag:"~num-nodes" desc:"For use with gcloud commands to specify the number of nodes for the cluster."` | |||
| ImageType string `flag:"~image-type" desc:"The image type to use for the cluster."` | |||
| ReleaseChannel string `desc:"Use a GKE release channel, could be one of empty, rapid, regular and stable - https://cloud.google.com/kubernetes-engine/docs/concepts/release-channels"` | |||
| Version string `desc:"Use a specific GKE version e.g. 1.16.13.gke-400, 'latest' or ''. If --build is specified it will default to building kubernetes from source."` | |||
| ClusterVersion string `desc:"Use a specific GKE version e.g. 1.16.13.gke-400, 'latest' or ''. If --build is specified it will default to building kubernetes from source."` | |||
| LegacyClusterVersion string `flag:"~version,deprecated" desc:"Use --cluster-version instead"` | |||
There was a problem hiding this comment.
most likely in the release afterwards. (v0.2.0)
| metadataJSON, err := os.Create( | ||
| filepath.Join(opts.RunDir(), "metadata.json"), | ||
| ) |
There was a problem hiding this comment.
Any reason this can't match the behavior of pkg.testers.metadata.WriteVersionToMetadata and preserve an existing metadata if it happens to exist?
There was a problem hiding this comment.
It can, but atleast at this point kubetest2 itself doesn't expect any metadata.json to be written out to the run specific output directory by itself.
| // DeployerWithVersion allows the deployer to specify it's version | ||
| type DeployerWithVersion interface { | ||
| Deployer | ||
|
|
||
| // Version determines the version of the deployer binary | ||
| Version() string | ||
| } | ||
|
|
There was a problem hiding this comment.
What would be involved in making Version() part of Deployer? Seeing three different DeployerWithFoo interfaces makes me wonder if it keeps things simpler to fold into core
There was a problem hiding this comment.
I think that's something we can add in v0.2.0
Currently, all out of tree deployers won't have Version() and will break the core interface.
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amwat, BenTheElder, spiffxp 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 |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
also add support for writing
metadata.jsonsimilar to what kubetest does in existing prow jobs.fixes: #108
For deployers:
the shim GitTag won't be present for out-of-tree providers instead inject an env
KUBETEST2_VERSIONfrom the shim to the deployers.For testers:
Went the route of overwriting metadata in testers.
alternative was to enforce all testers support a
--versionflag and detect it similar to--help, but is unnecessarily restrictive./cc @BenTheElder @spiffxp