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

Split plugin running #91

Merged
merged 33 commits into from
Jul 3, 2023
Merged

Split plugin running #91

merged 33 commits into from
Jul 3, 2023

Conversation

mfleader
Copy link
Member

Changes introduced with this PR

  • Split the plugin provider's Running Stage into a Starting Stage and a Running Stage.
  • Add unit tests

By contributing to this repository, I agree to the contribution guidelines.

mfleader added 23 commits June 16, 2023 09:52
@mfleader mfleader removed the request for review from jdowni000 June 20, 2023 18:48
@dustinblack
Copy link
Member

It looks like your 'remove dead code' commit may have left some artifacts that need cleaning up for the CI (or perhaps that code wasn't quite dead yet?).

@mfleader mfleader added the enhancement New feature or request label Jun 21, 2023
@dustinblack dustinblack linked an issue Jun 26, 2023 that may be closed by this pull request
Split plugin running stage #78
Closed
jdowni000
jdowni000 approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@jdowni000 jdowni000 left a comment

Choose a reason for hiding this comment

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

All tests pass. I like the additional comments, and the stages seemed to be split correctly as far as I can tell. Great job.

jaredoconnell
jaredoconnell requested changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@jaredoconnell jaredoconnell 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 overall. I agree with a lot of your design choices.

My test workflows worked properly with this branch. Some of my other changes tainted the environment making it so I can't run some test cases.

The only things that need fixing are the NextStages issue, and comments on basically all of the test cases that explain the high level goal of the test case, and the sections in the larger test cases. These comments will make it easier to review.

internal/step/plugin/provider_test.go Show resolved Hide resolved
internal/step/plugin/provider_test.go Show resolved Hide resolved
@jaredoconnell jaredoconnell mentioned this pull request Jul 3, 2023
Open
jaredoconnell
jaredoconnell approved these changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@jaredoconnell jaredoconnell 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 to me, except for the tests' documentation. I moved that to an issue: #96

@dustinblack dustinblack enabled auto-merge (squash) July 3, 2023 17:58
@mfleader mfleader force-pushed the split-plugin-running branch from 6e85055 to 6b2ee05 Compare July 3, 2023 17:59
@dustinblack dustinblack disabled auto-merge July 3, 2023 18:00
@dustinblack dustinblack enabled auto-merge (squash) July 3, 2023 18:04
@dustinblack dustinblack merged commit d606932 into main Jul 3, 2023
@dustinblack dustinblack deleted the split-plugin-running branch July 3, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Harshith-umesh Harshith-umesh Harshith-umesh left review comments

@jaredoconnell jaredoconnell jaredoconnell approved these changes

@jdowni000 jdowni000 jdowni000 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split plugin running stage
5 participants
@mfleader @dustinblack @Harshith-umesh @jaredoconnell @jdowni000