-
Notifications
You must be signed in to change notification settings - Fork 148
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
Improve change detection for environments, expand unit tests #646
Closed
Brad-Abrams
wants to merge
5
commits into
github:main-enterprise
from
RBC:Expand-unit-testing-for-environments
Closed
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
658e3e3
decompose unit tests, patch sync for environments
Brad-Abrams e8e7e53
remove logging, combine loops as per review comments
Brad-Abrams 36a39c1
Merge branch 'main-enterprise' into Expand-unit-testing-for-environments
Brad-Abrams cf3fa72
Add NopCommand, log.error, and errors
Brad-Abrams caa7d04
const log
Brad-Abrams File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
'NopCommand' is not defined here
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.
Thanks @klutchell . I have added a pair of commits to address this: cf3fa72 and caa7d04
With these additions, the NopCommand is now defined. Additionally, there was an argument missing from the constructor for the environments plugin when instantiated from the unit tests. That has also been corrected.
I appreciate that you're taking a look at the PR. I have more to follow on from this one.
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.
Any improvements to the environments plugin at this point is appreciated, as we are struggling to use this project to set environment level variables.
With PR #616 we were able to get it to create the initial set of environments and the respective variables, but now we can't get it to detect changes to those variables in the configuration.
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 experience suggests that this PR #646 is going to help with the detection and setting of environment level variables. That being said, there are remaining quirks in the environments plugin that I'd still like to address. For one, I can say that I appreciate the defensive code that you've added in PR# 616 to only post updates for variables and deployment protection rules if they are defined. My plan has been to see if I can succeed in getting this succinctly scoped PR reviewed and merged before layering in additional improvements. Am I on the right track? Which one of us is going to take a swing at combining 616 and 646? How would you feel about me taking it on? ... or would the resulting PR be too bloated?
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.
I'm not the project maintainer, but that sounds like the correct approach to me.
WDYT @decyjphr ?