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

Convert cmd/secrets-provider to unit testable entrypoint package #507

Merged
merged 7 commits into from
Mar 31, 2023

Conversation

john-odonnell
Copy link
Contributor

@john-odonnell john-odonnell commented Mar 16, 2023

Desired Outcome

  • Make main package unit testable.
  • Create program entrypoint where we can setup dependency injection.

Implemented Changes

  • conjur package:
    • Privatized SecretRetriever struct
    • Function NewSecretRetriever now returns type RetrieveSecretsFunc instead of struct SecretRetriever
    • New RetrieverFactory function type, which is implemented by NewSecretRetriever()
      // RetrieverFactory defines a function type for creating a SecretRetriever
      // implementation given an authenticator config.
      type RetrieverFactory func(authnConfig config.Configuration) (RetrieveSecretsFunc, error)
  • secrets package:
    • New ProviderFactory function type, which is implemented by NewProviderForType()
      // ProviderFactory defines a function type for creating a ProviderFunc given a
      // RetrieveSecretsFunc and ProviderConfig.
      type ProviderFactory func(traceContent context.Context, secretsRetrieverFunc conjur.RetrieveSecretsFunc, providerConfig ProviderConfig) (ProviderFunc, []error)
    • Refactored RepeatableSecretProvider, which returns a function that wraps a given ProviderFunc in logic to run Secrets Provider in one of its operating modes. Now, this function is called RunSecretsProvider, and the logic is executed instead of returned.
    • New StatusUpdaterFactory function type
      // StatusUpdaterFactory defines a function type for creating a StatusUpdater
      // implementation.
      type StatusUpdaterFactory func() StatusUpdater
    • New function NewStatusUpdater(), which implements StatusUpdaterFactory with default values.
  • Moved content of cmd/secrets-provider/* to new entrypoint package.
    • New function startSecretsProviderWithDeps():
      func StartSecretsProviderWithArguments(
          annotationsFilePath string,
          secretsBasePath string,
          templatesBasePath string,
          retrieverFactory conjur.RetrieverFactory,
          providerFactory secrets.ProviderFactory,
          statusUpdater secrets.StatusUpdaterFactory,
      ) (exitCode int)
    • New function StartSecretsProvider(), which calls startSecretsProviderWithDeps() with default values

Connected Issue/Story

CNJR-84

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: [insert issue ID]
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

pkg/entrypoint/entrypoint.go Outdated Show resolved Hide resolved
pkg/entrypoint/entrypoint.go Show resolved Hide resolved
pkg/entrypoint/entrypoint.go Outdated Show resolved Hide resolved
pkg/entrypoint/entrypoint.go Show resolved Hide resolved
@john-odonnell john-odonnell marked this pull request as ready for review March 17, 2023 16:49
@john-odonnell john-odonnell requested a review from a team as a code owner March 17, 2023 16:49
@szh
Copy link
Contributor

szh commented Mar 20, 2023

@john-odonnell are we going to make changes to make CC happy or just mark these as wontfix?

@john-odonnell
Copy link
Contributor Author

@szh Refactored repeatableSecretsProvider(), but will mark the rest as wontfix.

pkg/entrypoint/entrypoint.go Show resolved Hide resolved
pkg/entrypoint/entrypoint.go Outdated Show resolved Hide resolved
pkg/entrypoint/entrypoint.go Outdated Show resolved Hide resolved
szh
szh previously approved these changes Mar 21, 2023
Copy link
Contributor

@szh szh left a comment

Choose a reason for hiding this comment

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

LGTM

@john-odonnell john-odonnell force-pushed the johnodon-entrypoint branch 2 times, most recently from 374c7d8 to a2f9c20 Compare March 30, 2023 02:39
pkg/entrypoint/entrypoint.go Show resolved Hide resolved
pkg/entrypoint/entrypoint.go Show resolved Hide resolved
pkg/entrypoint/entrypoint.go Show resolved Hide resolved
os.Exit(exitCode)
}

func startSecretsProviderWithDeps(
Copy link

Choose a reason for hiding this comment

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

Function startSecretsProviderWithDeps has 51 lines of code (exceeds 50 allowed). Consider refactoring.

- Remove defaultStatusUpdater singleton in favor of NewStatusUpdater factory.
- Convert RepeatableSecretsProvider, which returns a RepeatableProviderFunc,
  to RunSecretsProvider, which runs execution logic.
- ./bin/test_unit passes all args to go test command.
@codeclimate
Copy link

codeclimate bot commented Mar 31, 2023

Code Climate has analyzed commit 2d70273 and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 4

The test coverage on the diff in this pull request is 82.9% (50% is the threshold).

This pull request will bring the total coverage in the repository to 88.2% (-1.0% change).

View more on Code Climate.

Copy link
Contributor

@doodlesbykumbi doodlesbykumbi left a comment

Choose a reason for hiding this comment

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

LGTM! Truly a journey of a thousand miles, thanks for nudging the refactor effort in a good direction

bin/test_unit Outdated
Comment on lines 24 to 27
test_case_cmd=""
if [[ "$test_case" != "" ]]; then
test_case_cmd="--run $test_case"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on making this more open by just passing along all arguments to this test_unit ... script to go test ...

@john-odonnell john-odonnell merged commit 7368fcc into main Mar 31, 2023
@john-odonnell john-odonnell deleted the johnodon-entrypoint branch March 31, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants