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

Refactor Trigger Actions and Add Ability To Kick Off Runs With Specified Terraform Version #20

Merged
merged 3 commits into from
Jan 31, 2023
Merged

Refactor Trigger Actions and Add Ability To Kick Off Runs With Specified Terraform Version #20

merged 3 commits into from
Jan 31, 2023

Conversation

mplachter
Copy link
Collaborator

  • Work in Progress trigger_actions refactor
    • Why
      • Simplify implementation and API
      • Make it easier to add arguments to pass back to trigger_actions
    • Pending
      • fix mocks
      • fix unit tests
  • Pass TF Version to overwrite run with diff terraform version
    • good from a testing perspective

@mplachter
Copy link
Collaborator Author

I will rebase -i and sign this later this is still a WIP leaving in Draft for now

@mplachter mplachter marked this pull request as ready for review January 27, 2023 16:45
@mplachter
trigger_actions refactor and terraform version plan testing for upgra…
e85c22f 
…dability

Signed-off-by: Matt Plachter <matthew.plachter@zapier.com>

refactored options and mock tests

add ability to pass back tfversion to run

fixed functionality for setting version during plans only for upgrade testing.

removed commented out mocks from left over config

regenerated mocks with updated interface

minor doc additions
@mplachter mplachter force-pushed the allow-tf-buddy-to-kick-off-specific-tf-workspace-version branch from 004f1d6 to e85c22f Compare January 27, 2023 16:48
@mplachter mplachter changed the title DRAFT: Refactor Trigger Actions and Add Ability To Kick Off Runs With Specified Terraform Version Refactor Trigger Actions and Add Ability To Kick Off Runs With Specified Terraform Version Jan 27, 2023
davidwin93
davidwin93 reviewed Jan 31, 2023
View reviewed changes
README.md Outdated
@@ -27,6 +27,10 @@ TFBuddy consists of the webhook handler and a NATS cluster.

See [Installation Docs](https://tfbuddy.readthedocs.io/en/stable/usage/)

## Contributing

The [contributing](./docs/contributing.md) has everything you need to start working on TFBuddy.
Copy link
Collaborator

Choose a reason for hiding this comment

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

mind linking this to read the docs instead?

pkg/comment_actions/parsing.go Outdated
"github.com/zapier/tfbuddy/pkg/utils"
)

var (
ErrNotTFCCommand = errors.New("not a TFC command")
ErrOtherTFTool = errors.New("Use 'tfc' to interact with tfbuddy.")
ErrOtherTFTool = errors.New("use 'tfc' to interact with tfbuddy")
ErrNoNotePassed = errors.New("no notes passed in not block")
Copy link
Collaborator

Choose a reason for hiding this comment

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

note instead of not

pkg/comment_actions/parsing.go Show resolved Hide resolved
pkg/comment_actions/parsing_test.go Show resolved Hide resolved
sl1pm4t
sl1pm4t suggested changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@sl1pm4t sl1pm4t left a comment

Choose a reason for hiding this comment

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

Looks good, a useful new feature and some great code cleanup.
Just a couple of nits, to cleanup some commented out code.

pkg/mocks/helpers.go Outdated
@@ -225,7 +211,7 @@ func CreateTestSuite(mockCtrl *gomock.Controller, overrides TestOverrides, t *te
mockGitDisc := NewMockMRDiscussionNotes(mockCtrl)
mockMRNote := NewMockMRNote(mockCtrl)

mockTriggerConfig := NewMockTriggerConfig(mockCtrl)
// mockTriggerConfig := NewMockTriggerConfig(mockCtrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this now?

pkg/mocks/helpers.go Outdated
MockGitRepo: mockGitRepo,
MockGitDisc: mockGitDisc,
MockMRNote: mockMRNote,
// MockTriggerConfig: mockTriggerConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment

pkg/mocks/helpers.go Outdated
MockGitRepo *MockGitRepo
MockGitDisc *MockMRDiscussionNotes
MockMRNote *MockMRNote
// MockTriggerConfig *MockTriggerConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// MockTriggerConfig *MockTriggerConfig

@mplachter
Fixes for MR comments
b1c58ea 
Signed-off-by: Matt Plachter <matthew.plachter@zapier.com>

redirect to contrib url

fix spelling error

remove commented out code
@mplachter mplachter force-pushed the allow-tf-buddy-to-kick-off-specific-tf-workspace-version branch from 14dbccd to b1c58ea Compare January 31, 2023 18:03
davidwin93
davidwin93 reviewed Jan 31, 2023
View reviewed changes
pkg/gitlab_hooks/mr_actions.go
})
if err != nil {
log.Error().Err(err).Msg("could not create TFCTriggerConfig")
return projectName, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should wrap this as a Permanent Error unless it can recovered. utils.CreatePermanentError since that'll put it back on the Nats queue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes 100p sense. i moved it into the NewTFCTriggerConfig function so the error will always be wrapped with the permanent error. 👍

@mplachter
Make NewTFCTriggerConfig return a PermanentError
c84ce8f 
Signed-off-by: Matt Plachter <matthew.plachter@zapier.com>
@mplachter mplachter merged commit d24ed6f into main Jan 31, 2023
@sl1pm4t sl1pm4t deleted the allow-tf-buddy-to-kick-off-specific-tf-workspace-version branch July 7, 2023 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sl1pm4t sl1pm4t sl1pm4t approved these changes

@davidwin93 davidwin93 davidwin93 approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@mplachter @sl1pm4t @davidwin93