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

Upgrade command #659

Merged
merged 19 commits into from
Jul 31, 2019
Merged

Upgrade command #659

merged 19 commits into from
Jul 31, 2019

Conversation

alenkacz
Copy link
Contributor

@alenkacz alenkacz commented Jul 26, 2019

@alenkacz alenkacz requested a review from gerred as a code owner July 26, 2019 11:15
@alenkacz alenkacz changed the title PoC of upgrade command Upgrade command (PoC) Jul 26, 2019
@alenkacz alenkacz changed the title Upgrade command (PoC) WIP: Upgrade command Jul 29, 2019
@alenkacz alenkacz changed the title WIP: Upgrade command Upgrade command Jul 29, 2019
if ov == nil {
return fmt.Errorf("no operator version for this operator installed yet for %s in namespace %s. Please use install command if you want to install new operator into cluster", operatorName, options.Namespace)
}
oldVersion, _ := semver.NewVersion(ov.Spec.Version)

Choose a reason for hiding this comment

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

Should we be suppressing this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no :)

@jbarrick-mesosphere
Copy link
Member

/kind feature

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

There are some small things to resolve.
My biggest concern is an "upgrade" is in part an "install" with the condition that an instance of a version of the operator is present... with that context in mind... I don't see how params will be resolved....

  1. Do we use previous values (or can we)
  2. I do expect that future versions may have different or added params

I'm expecting that we have to have -p, --parameter stringArray for upgrade.

pkg/kudoctl/util/kudo/kudo.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/upgrade.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/upgrade.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/upgrade.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/upgrade.go Outdated Show resolved Hide resolved
@alenkacz alenkacz added this to the 0.5.0 milestone Jul 30, 2019
@kudo-ci kudo-ci removed the size/L label Jul 30, 2019
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

love it! really nice work! there are some minor changes needed... most significant is the CLI output for default namespace.

@@ -49,26 +49,3 @@ func TestTableNewInstallCmd_WithParameters(t *testing.T) {
assert.NotNil(t, err, test.errorMessage)
}
}

var parameterParsingTests = []struct {
Copy link
Member

Choose a reason for hiding this comment

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

these tests disappeared... shouldn't they move to params_test.go ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it got moved to params_test. Oh wait, I did not push that, LOL :D

pkg/kudoctl/cmd/upgrade.go Outdated Show resolved Hide resolved
return upgradeCmd
}

func validate(args []string, options *options) error {
Copy link
Member

Choose a reason for hiding this comment

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

this feels more like a validateCmd or validateArgs... find a validate in upgrade I was thinking validation rules around upgrade which this isn't. much of that is in upgrade where version comparisons are done.

simply mentioning... it is a a weak comment on my part... it looks fine just what was in my head as I reviewed.

pkg/kudoctl/cmd/upgrade.go Outdated Show resolved Hide resolved
@kudo-ci
Copy link

kudo-ci commented Jul 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alenkacz, kensipe

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kensipe
Copy link
Member

kensipe commented Jul 31, 2019

/lgtm

@kudo-ci kudo-ci added the lgtm label Jul 31, 2019
@kudo-ci kudo-ci merged commit 5e2dfd0 into master Jul 31, 2019
@kudo-ci kudo-ci deleted the av/upgrade-command branch July 31, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable upgrades through KUDO cli
4 participants