Improve change detection for environments, expand unit tests #646
Conversation
| return Promise.resolve(resArray) | ||
| } else { | ||
| resArray.push(new NopCommand(this.constructor.name, this.repo, null, `error ${e} in ${this.constructor.name} for repo: ${JSON.stringify(this.repo)} entries ${JSON.stringify(this.entries)}`, 'ERROR')) |
There was a problem hiding this comment.
'NopCommand' is not defined here
There was a problem hiding this comment.
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.
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.
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.
I'm not the project maintainer, but that sounds like the correct approach to me.
WDYT @decyjphr ?
|
I'm going to give this PR a try, with some minor changes from #616 on top. |
|
I have a new PR inbound that will combine together this PR #646 and Kyle's PR #616. It just needs to navigate some internal processes first. It will additionally address issue #623 with a documentation update. I also noticed that PR #630 relates to environments, however it has no material code change and so I've not taken it into consideration. ... I read it some more, and now I've taken it into consideration ... PR #630 stated that it was intended to address issue #628. I'm going to see what I can do about #628 |
This commit combines PR [616](github#616) and [646](github#646) environments.js Add defensive code to prevent the GitHub API from being called with undefined data. In the UI, and API an environment can be added with just an name. Now, safe-settings permits this as well. In the UI, and API an environment can be added without variables. Now, safe-settings permits this as well. In the UI, and API an environment can be added without deployment_protection_rules. Now, safe-settings permits this as well. environments.test.js Add a test case for the scenario when there are zero existing environments Add a test case for an environment name change Add a test case inspired by PR 616 which adds 7 new environments with various attributes Move expect statements out of aftereach() as there is now variability in what is expected across test cases. Specifically, when there is no existing environment, that environment should NOT be queried for variables nor deployment_protection_rules
|
@Brad-Abrams @klutchell We also found issue with adding or modifying environment variables second time once they are created. Looks like issue is with mergeDeep.js logic. I am also working to fix it. However, I have created issue for the same. |
|
@luvsaxena1 that issue aligns with what we are seeing, both changed variables and updated rulesets are not detected by mergeDeep. |
|
@klutchell @Brad-Abrams I have created PR to address merge deep code issues. I believe changes, I made, will successfully detect changes to the existing environment variables and other settings. |
* decompose unit tests, patch sync for environments * remove logging, combine loops as per review comments * Add NopCommand, log.error, and errors * Allow concise config for Environments This commit combines PR [616](#616) and [646](#646) environments.js Add defensive code to prevent the GitHub API from being called with undefined data. In the UI, and API an environment can be added with just an name. Now, safe-settings permits this as well. In the UI, and API an environment can be added without variables. Now, safe-settings permits this as well. In the UI, and API an environment can be added without deployment_protection_rules. Now, safe-settings permits this as well. environments.test.js Add a test case for the scenario when there are zero existing environments Add a test case for an environment name change Add a test case inspired by PR 616 which adds 7 new environments with various attributes Move expect statements out of aftereach() as there is now variability in what is expected across test cases. Specifically, when there is no existing environment, that environment should NOT be queried for variables nor deployment_protection_rules * Update documentation: Environments permissions. Addresses issue: [Environments do not get provisioned for repositories set to internal or private #623](#623) Adds documentation for permissions required for safe-settings when Environments are used [List Environments](https://docs.github.com/en/rest/deployments/environments?apiVersion=2022-11-28#list-environments) API requires: ``` The fine-grained token must have the following permission set: "Actions" repository permissions (read) ``` [Create an environment variable](https://docs.github.com/en/rest/actions/variables?apiVersion=2022-11-28#create-an-environment-variable) API requires: ``` The fine-grained token must have the following permission set: "Variables" repository permissions (write) and "Environments" repository permissions (write) ``` With permissions added, issue 623 was resolved.
|
With PR #649 having been approved and merged, this PR is now redundant. Closing. |
* decompose unit tests, patch sync for environments * remove logging, combine loops as per review comments * Add NopCommand, log.error, and errors * Allow concise config for Environments This commit combines PR [616](github#616) and [646](github#646) environments.js Add defensive code to prevent the GitHub API from being called with undefined data. In the UI, and API an environment can be added with just an name. Now, safe-settings permits this as well. In the UI, and API an environment can be added without variables. Now, safe-settings permits this as well. In the UI, and API an environment can be added without deployment_protection_rules. Now, safe-settings permits this as well. environments.test.js Add a test case for the scenario when there are zero existing environments Add a test case for an environment name change Add a test case inspired by PR 616 which adds 7 new environments with various attributes Move expect statements out of aftereach() as there is now variability in what is expected across test cases. Specifically, when there is no existing environment, that environment should NOT be queried for variables nor deployment_protection_rules * Update documentation: Environments permissions. Addresses issue: [Environments do not get provisioned for repositories set to internal or private github#623](github#623) Adds documentation for permissions required for safe-settings when Environments are used [List Environments](https://docs.github.com/en/rest/deployments/environments?apiVersion=2022-11-28#list-environments) API requires: ``` The fine-grained token must have the following permission set: "Actions" repository permissions (read) ``` [Create an environment variable](https://docs.github.com/en/rest/actions/variables?apiVersion=2022-11-28#create-an-environment-variable) API requires: ``` The fine-grained token must have the following permission set: "Variables" repository permissions (write) and "Environments" repository permissions (write) ``` With permissions added, issue 623 was resolved.
This PR is superseded by PR #649
Environments unit test hid an error in Environments plugin; fixed
TLDR; removes a redundant/faulty check for changes in the environments plugin
Before:
Before this change there was a single unit test for environments. That test modelled 7 environments with a change in each.
Two of the seven changes (wait_timer, prevent_self_review) were detected by mergeDeep.compareDeep(); 5 were not detected.
The two changes that were detected were enough to lift the code execution past this point, deeper into the environments code which itself again uses it's own comparator() where this time it successfully finds the 7 changes.
It was theorized that if the unit test was decomposed into it's seven constituent pieces that only 2/7 would pass. This turned out to be true and is a better match to our lived experience with using environments in safe-settings:
Now on the theory that the first round of change detection was redundant, it was removed. This was accomplished by copying the sync method from diffable.js into environments.js and then removing the lines which failed to detect the changes. A new run of the unit tests, post change, passes 7/7:
But, what if the removed code wasn't really redundant? What if the existing config, and the desired config were the same; would environments.js now erroneously call GitHub to make a change when none was required? Nope:
After:
The original test with 7 environments and 7 changes bundled together was added back in alongside the decomposed tests. It also passes. Here are the final results:
Request:
I would be most appreciative of a review and merge of this change. I am available for any follow up questions which may arise.