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.

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

Environments tolerate concise configuration #649

Merged

Conversation

Brad-Abrams
Copy link
Contributor

A basket of improvements to Environments in safe-settings.

The GitHub UI and APIs permit the creation of an environment with only a name. Before this change the environments plugin for safe-settings required a verbose definition of environments. This boiled down to failing api calls that were being made even if the config had empty values. Environments can now be modelled without the need to set empty arrays for variables and deployment protection rules.

The largest change in this PR is to the unit tests for Environments. One all encompassing test has been broken down into smaller units as detailed with screenshots in PR #646. Additionally, tests have been added for a previously missing scenario where there were zero environments existing in the repo.

This PR with commit fc179df combines together PR #646 and PR #616. I suggest that if this PR is taken, then other two can be closed without merging. However @klutchell would be the authority to speak to 616.

This PR with commit 8d369c0 addresses issue #623 via documentation updates which add in previously missing required permissions when using safe-settings with Environments.

I would be most appreciative of a review and subsequent merge.

Brad-Abrams and others added 7 commits May 14, 2024 15:36
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
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.
@gregnrobinson
Copy link

I can confirm that the documentation changes for additional permissions address the issue of managing environments under private or internal repositories.

I can also confirm that these changes address the API errors received when creating and managing existing environments under safe-settings.

We should get these changes reviewed and merged ASAP.

@klutchell
Copy link
Contributor

I don't think #616 is required if we can get this merged

@luvsaxena1
Copy link
Contributor

luvsaxena1 commented Aug 14, 2024

@Brad-Abrams Is your PR addressed this issue as well ?
#551

This is another problem with environment in safe settings

@Brad-Abrams
Copy link
Contributor Author

@Brad-Abrams Is your PR addressed this issue as well ? #551

This is another problem with environment in safe settings

I did not do anything to try to address #551. That felt like it was deserving of a separate PR.

@Brad-Abrams
Copy link
Contributor Author

A side note that this PR also addresses an issue submitted after this was created #660

Additionally, I will note that this PR and PR #665 are compatible. Taking both is my recommendation.

@Brad-Abrams
Copy link
Contributor Author

@decyjphr I've been a grateful user of safe-settings for many months now, and would like to also be a contributor. Please consider this PR to tighten up the handling of environments.

Copy link
Collaborator

@decyjphr decyjphr left a comment

Choose a reason for hiding this comment

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

LGTM

@decyjphr decyjphr merged commit 19e2691 into github:main-enterprise Aug 15, 2024
@Brad-Abrams
Copy link
Contributor Author

Thank you kindly @decyjphr, @klutchell, @luvsaxena1 and those behind the scenes. I appreciate the support in seeing this through. Expect to see me again for further safe-settings contributions.

admtorgst pushed a commit to helse-sorost/safe-settings that referenced this pull request Sep 14, 2024
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants