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.

Sign up for GitHub

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

Jump to bottom

Allow FileTriggersEnabled to be set to false when Git tags are present #468

Merged
merged 4 commits into from
Aug 10, 2022
Merged

Allow FileTriggersEnabled to be set to false when Git tags are present #468

merged 4 commits into from
Aug 10, 2022

Conversation

hashimoon
Copy link
Contributor

@hashimoon hashimoon commented Jul 16, 2022 •
edited
Loading

Description

File triggers must be disabled when enabling Git tags, so both FileTriggersEnabled and TagsRegex need to support being configured at the same time.

Testing plan

Creating A Workspace

  1. Create a WorkspaceCreateOptions struct
  2. Set FileTriggersEnabled to true
  3. Create a VCSRepoOptions struct
  4. Set TagsRegex to "v-1"
  5. On the WorkspaceCreateOptions struct, set VCSRepo equal to VCSRepoOptions
  6. Verify that an exception is raised
  7. Set FileTriggersEnabled to false
  8. Verify an exception is not raised

Updating A Workspace

  1. Create a WorkspaceUpdateOptions struct
  2. Set FileTriggersEnabled to true
  3. Create a VCSRepoOptions struct
  4. Set TagsRegex to "v-1"
  5. On the WorkspaceUpdateOptions struct, set VCSRepo equal to VCSRepoOptions
  6. Verify that an exception is raised
  7. Set FileTriggersEnabled to false
  8. Verify an exception is not raised

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" TF_ACC="1" go test ./... -v -tags=integration -run TestFunctionsAffectedByChange

...

@hashimoon hashimoon force-pushed the hashimoon/test-test branch 3 times, most recently from a9a5f59 to ce28630 Compare July 19, 2022 01:49
@hashimoon hashimoon changed the title wip Allow FileTriggersEnabled to be set to false when Git tags are present Jul 19, 2022
@hashimoon hashimoon marked this pull request as ready for review July 19, 2022 01:52
@hashimoon hashimoon requested a review from a team as a code owner July 19, 2022 01:52
@hashimoon hashimoon requested review from omarismail and a team July 19, 2022 01:52
matejrisek
matejrisek reviewed Jul 19, 2022
View reviewed changes
workspace.go Outdated
if o.VCSRepo != nil && o.VCSRepo.TagsRegex != nil &&
o.FileTriggersEnabled != nil && *o.FileTriggersEnabled {
if o.VCSRepo != nil && o.VCSRepo.TagsRegex != nil && *o.VCSRepo.TagsRegex != "" &&
o.FileTriggersEnabled != nil && *o.FileTriggersEnabled == true {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not just leave it as *o.FileTriggersEnabled

matejrisek
matejrisek reviewed Jul 19, 2022
View reviewed changes
workspace_integration_test.go
@@ -414,6 +413,18 @@ func TestWorkspacesCreate(t *testing.T) {
assert.EqualError(t, err, ErrUnsupportedBothTagsRegexAndFileTriggersEnabled.Error())
})

t.Run("when options include both non-empty tags-regex and file-triggers-enabled as false an error is not returned", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: We could use table driven tests and merge together this and the above 3 tests

Copy link
Contributor

Choose a reason for hiding this comment

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

@hashimoon We're good with adding table driven tests here - agree that it would help readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the rest of the file is not utilizing table driven tests and the style is consistent with what is already present, can we add them in a future refactor? @annawinkler

matejrisek
matejrisek reviewed Jul 19, 2022
View reviewed changes
workspace_integration_test.go
@@ -890,6 +900,18 @@ func TestWorkspacesUpdate(t *testing.T) {
assert.EqualError(t, err, ErrUnsupportedBothTagsRegexAndFileTriggersEnabled.Error())
})

t.Run("when options include both non-empty tags-regex and file-triggers-enabled an error is returned", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one could be a table driven test

annawinkler
annawinkler suggested changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

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

I'd like to clarify with the CLI team how we'd like to handle beta features because I think there have been some inconsistencies in the past. It was my understanding we don't want to include features until they are GA.

Could you please add a few steps to the test plan so someone unfamiliar with this change can test/verify locally? Thank you! ⭐

@hashimoon hashimoon force-pushed the hashimoon/test-test branch 2 times, most recently from 5f392a8 to 2fa1542 Compare July 19, 2022 16:31
annawinkler
annawinkler reviewed Jul 19, 2022
View reviewed changes
annawinkler
annawinkler reviewed Jul 19, 2022
View reviewed changes
@hashimoon hashimoon marked this pull request as draft July 20, 2022 18:20
@mjyocca mjyocca force-pushed the hashimoon/test-test branch from 07c9486 to 6e9d327 Compare August 10, 2022 14:34
@mjyocca mjyocca requested a review from annawinkler August 10, 2022 14:34
@mjyocca mjyocca marked this pull request as ready for review August 10, 2022 14:36
@mjyocca mjyocca mentioned this pull request Aug 10, 2022
Merged
annawinkler
annawinkler approved these changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

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

:shipit: I'm OK with refactoring the tests in a separate PR.

@mjyocca mjyocca merged commit f8bcba2 into main Aug 10, 2022
@mjyocca mjyocca deleted the hashimoon/test-test branch August 10, 2022 21:43
@github-actions
Copy link

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejrisek matejrisek matejrisek left review comments

@annawinkler annawinkler annawinkler approved these changes

@omarismail omarismail Awaiting requested review from omarismail

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hashimoon @annawinkler @matejrisek @mjyocca