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

Add various functional options to ActionConfigGetter and ActionClientGetter constructors #196

Merged

Conversation

joelanford
Copy link
Member

@joelanford joelanford commented Aug 29, 2022

This PR adds

In ActionConfigGetter constructor:

  • Use a custom mapper in the action config getter to map an object to the helm client's configured namespace
  • Use a custom mapper in the action config getter to map an object to the namespace where the release secret is stored
  • Configure whether or not the release secret will have an owner reference to the object for which the action.Configuration is generated.

These are useful to give users of this library more control to decouple release storage from install namespace.

In ActionClientGetter constructur:

  • Append custom default functional options used to create action.Get, action.Install, action.Upgrade, action.Uninstall, and action.Rollback.

These are useful to give users of this library the ability to customize how the various methods of the default ActionInterface interact with Helm. One example use case here is setting Wait to true for uninstall/rollback to ensure that subsequent reconciliations do not encounter conflicting objects that are still being deleted.

Signed-off-by: Joe Lanford joe.lanford@gmail.com

@coveralls
Copy link

coveralls commented Aug 29, 2022

Pull Request Test Coverage Report for Build 3532484677

  • 109 of 117 (93.16%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 89.498%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/client/actionclient.go 65 67 97.01%
pkg/reconciler/reconciler.go 2 4 50.0%
pkg/client/actionconfig.go 42 46 91.3%
Totals Coverage Status
Change from base Build 3445575816: 0.3%
Covered Lines: 1713
Relevant Lines: 1914

💛 - Coveralls

@joelanford joelanford force-pushed the configurable-actionconfiggetter branch from 14b86b4 to ac91362 Compare September 16, 2022 12:48
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2022
@joelanford joelanford force-pushed the configurable-actionconfiggetter branch from ac91362 to 2b2e7c8 Compare September 17, 2022 01:47
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 17, 2022

New changes are detected. LGTM label has been removed.

@joelanford joelanford force-pushed the configurable-actionconfiggetter branch from 2b2e7c8 to b6678ec Compare September 17, 2022 02:50
@joelanford joelanford force-pushed the configurable-actionconfiggetter branch from b6678ec to 19764b2 Compare September 19, 2022 21:14
@joelanford joelanford force-pushed the configurable-actionconfiggetter branch from 19764b2 to 286a3a9 Compare November 11, 2022 12:56
@joelanford joelanford changed the title Add various functional options to ActionClientGetter constructor Add various functional options to ActionConfigGetter and ActionClientGetter constructors Nov 11, 2022
@joelanford joelanford force-pushed the configurable-actionconfiggetter branch 2 times, most recently from 932d72c to a4d37a8 Compare November 11, 2022 13:50
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Overall looks good to me.

Just have a couple questions/nits that are non-blocking (they can just be deemed irrelevant and we can move forward):

pkg/client/actionclient.go Show resolved Hide resolved
pkg/client/actionclient.go Show resolved Hide resolved
…Getter constructors

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford joelanford force-pushed the configurable-actionconfiggetter branch from a4d37a8 to 117a5df Compare November 23, 2022 13:29
@joelanford joelanford merged commit e688971 into operator-framework:main Nov 23, 2022
@joelanford joelanford deleted the configurable-actionconfiggetter branch November 23, 2022 13:41
Comment on lines +122 to +128
defaultGetOpts []GetOption
defaultInstallOpts []InstallOption
defaultUpgradeOpts []UpgradeOption
defaultUninstallOpts []UninstallOption

installFailureUninstallOpts []UninstallOption
upgradeFailureRollbackOpts []RollbackOption
Copy link
Member

Choose a reason for hiding this comment

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

This PR looks great overall. The only thing that keeps me wondering is that I suspect the lack/presence of the default prefix for these fields is trying to convey something, but I'm not sure what. Perhaps I'm just not familiar enough with the code...

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.

None yet

4 participants