-
Notifications
You must be signed in to change notification settings - Fork 10.1k
PSS: Add tests showing state subcommands being used with PSS
#37891
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the test will still need to build the appropriate binary and put it in a folder here that's named appropriately for the platform that is running the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I thought that go build creates the parent directories in the path, so this should not be necessary? I suppose it just means we need to cleanup the directory afterwards rather than just the binary.
|
Ooh, actually I'm going to add a non-E2E version of the tests to help with code coverage reporting, moving to draft. |
state subcommands being used with PSSstate show and state list subcommands being used with PSS
state show and state list subcommands being used with PSSstate show and state list subcommands being used with PSS
state show and state list subcommands being used with PSSstate subcommands being used with PSS
28e61ce to
cebf4a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The push command has its own test fixture, versus the shared internal/command/testdata/state-commands-state-store, because it needs a local state file to be the data that's pushed up to the state store.
radeksimko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only major question is about the gating of the E2E test, the rest is really minor/nitpicky stuff.
| t.Skip("can't run without building a new provider executable") | ||
| } | ||
|
|
||
| t.Setenv(e2e.TestExperimentFlag, "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we setting this to true? I feel like that defeats the original purpose of the flag/variable, which is to avoid running E2E tests by default during go test ./...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the e2e.GoBuild method on L140 builds the Terraform binary it uses this ENV to decide if the binary should be built with experiments enabled or not:
Lines 260 to 262 in 0941763
| if exp := os.Getenv(TestExperimentFlag); exp != "" && exp != "false" { | |
| args = append(args, "-ldflags", "-X 'main.experimentsAllowed=yes'") | |
| } |
It looks like this was added in the context of the Search project ~5 months ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I thought that go build creates the parent directories in the path, so this should not be necessary? I suppose it just means we need to cleanup the directory afterwards rather than just the binary.
…ing tests where the values in `MockStates` aren't compatible with the `ReadStateBytesFn` default function. Make existing test set an appropriate value in `MockStates`.
…h pluggable state storage code.
…h pluggable state storage code.
…h pluggable state storage code.
…on with pluggable state storage code.
…pluggable state storage code.
…pluggable state storage code.
…h pluggable state storage code.
…egration with pluggable state storage code.
bd6f62e to
3af7596
Compare
| mockProviderAddress := addrs.NewDefaultProvider("test") | ||
| providerSource, close := newMockProviderSource(t, map[string][]string{ | ||
| "hashicorp/test": {"1.0.0"}, | ||
| }) | ||
| defer close() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context: I realised that in lots of tests for PSS I've been adding this provider source value into the Meta in tests. Provider sources are only needed in the context of an init command, so providing them in these tests is unnecessary.
Similarly there are times where supplying both a View and Ui is unnecessary, and some commands need a Stream. I guess this reflects how different commands' terminal output is implemented slightly differently (shout out to #37439), and so only some outputs are required in their tests.
3af7596 to
bb7d519
Compare
…mand under test. This test fixure is reused across tests that need the config to define a state store but otherwise rely on the mock provider to set up the test scenario.
The internal/command/testdata/state-commands-state-store and internal/command/testdata/state-store-unchanged test fixtures are the same.
…using grpcwrap package This was removed, incorrectly, in #37899
…turned from `mockPluggableStateStorageProvider` and the `MockStates` that's been set in the mock
bb7d519 to
c2d9c42
Compare
Following #37569, this PR demonstrates that these commands work when pluggable state storage is in use:
state identitiesstate liststate mvstate replace-providerstate rmstate pullstate pushstate showThis PR mostly implements integration tests, but adds E2E tests for
state listandstate show.Target Release
N/A
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry