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

fix: Github Environments Policy feature causing the provider to produce inconsistent result #1799

Merged
merged 15 commits into from
Jul 24, 2023
Merged

fix: Github Environments Policy feature causing the provider to produce inconsistent result #1799

merged 15 commits into from
Jul 24, 2023

Conversation

edheliel
Copy link
Contributor

@edheliel edheliel commented Jul 19, 2023 •
edited

Resolves #922
Stale initial PR: #1530

Behavior

This PR is based on the now appearing to be stale PR #1530.

  • Fix the issue with the provider becoming inconsistent
    • It was cause by double encoding which made the environment name that contains a / wrong therefore not existing
    • use url.PathEscape since the github API operations use path parameters rather than query

Before the change?

  • envName was encoded 2 time which cause the Environment Branch Policy resource Read operation to not be able to find the file that was just created. That caused the provider to produce an inconsistent result
  • Usage of url.QueryEscape instead of url.PathEscape

After the change?

  • delete and read operation don't try to encode the envName a second time as they are receiving it already encoded and a second encoding breaks the name and url path
  • use url.PathEscape

Other information

Additional info

Most of the work was done on the original PR which seem to have become stale.
My motivation for creating this one is to unblock a feature that will allow for improvement in security when it comes to Github Actions Workflows and etc

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

No

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

Pull request type

Bug fix

ilmax and others added 12 commits March 17, 2023 19:46
@ilmax
Add deployment policy resource
e9d592c 
- Add the initial code to manage the resource
- Add sample configuration used to test it

TODO
- Documentation
- Tests
@ilmax
Add schema description
e845f87
@ilmax
Fix creation of resource ID
8c72108
@ilmax
Add tests
21c5dee
@ilmax
Add documentation
a3b21aa
@ilmax
Add terraform import support
bf38b3b
@ilmax
Undo example add
fc1a22b
@ilmax
Fix formatting
35321df
@ilmax
PR feedback
d487284
@kfcampbell
Merge branch 'main' into feat/environment-protection-rules
2acf03b
@kfcampbell
Merge branch 'main' into feat/environment-protection-rules
e09de38
@edheliel
fix: environment branch policy failing to find the created resource
d92a619 
The `Read` operation of the Environments Branch Policy resource
was failing to find the newly created Branch policies, due to
wrongly encoded environment name. Which cause the provider to
be inconsistent.

This fix uses `url.PathEscape` instead of `url.QueryEscape`
since we are using path parameters with the Github API in
that case. Additionally 2 operations - `Read` and `Delete`
don't need to use it as they receive the environment name
already parsed and attempting to encode it again breaks the
name.
@edheliel edheliel mentioned this pull request Jul 19, 2023
Closed
5 tasks
kfcampbell
kfcampbell approved these changes Jul 24, 2023
View reviewed changes
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

Thank you @edheliel and @ilmax for digging in here! I'll get this merged and released shortly.

@kfcampbell kfcampbell merged commit 64f123a into integrations:main Jul 24, 2023
3 checks passed
doonga pushed a commit to doonga/greyrock-ops that referenced this pull request Jul 25, 2023
@greyrock-repo-duster
feat(terraform-provider): update terraform github to 5.32.0 (#979)
5fab8b7 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github](https://registry.terraform.io/providers/integrations/github)
([source](https://togithub.com/integrations/terraform-provider-github))
| required_provider | minor | `5.31.0` -> `5.32.0` |

---

### Release Notes

<details>
<summary>integrations/terraform-provider-github (github)</summary>

###
[`v5.32.0`](https://togithub.com/integrations/terraform-provider-github/releases/tag/v5.32.0)

[Compare
Source](https://togithub.com/integrations/terraform-provider-github/compare/v5.31.0...v5.32.0)

#### What's Changed

- Add PR templates by
[@&#8203;nickfloyd](https://togithub.com/nickfloyd) in
[integrations/terraform-provider-github#1803
- feat: add data source to get organization members' SAML/SCIM linked
identities by [@&#8203;jsifuentes](https://togithub.com/jsifuentes) in
[integrations/terraform-provider-github#1778
- fix: Github Environments Policy feature causing the provider to
produce inconsistent result by
[@&#8203;edheliel](https://togithub.com/edheliel) in
[integrations/terraform-provider-github#1799
- Enforce valid chars in repo name by
[@&#8203;btlogy](https://togithub.com/btlogy) in
[integrations/terraform-provider-github#1806

#### New Contributors

- [@&#8203;edheliel](https://togithub.com/edheliel) made their first
contribution in
[integrations/terraform-provider-github#1799
- [@&#8203;btlogy](https://togithub.com/btlogy) made their first
contribution in
[integrations/terraform-provider-github#1806

**Full Changelog**:
integrations/terraform-provider-github@v5.31.0...v5.32.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4yMy4wIiwidXBkYXRlZEluVmVyIjoiMzYuMjMuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: greyrock-bot <112729691+greyrock-bot[bot]@users.noreply.github.com>
o-sama pushed a commit to o-sama/terraform-provider-github that referenced this pull request Aug 19, 2023
@edheliel @ilmax
@kfcampbell @o-sama
fix: Github Environments Policy feature causing the provider to produ…
671cbc2 
…ce inconsistent result (integrations#1799)

* Add deployment policy resource

- Add the initial code to manage the resource
- Add sample configuration used to test it

TODO
- Documentation
- Tests

* Add schema description

* Fix creation of resource ID

* Add tests

* Add documentation

* Add terraform import support

* Undo example add

* Fix formatting

* PR feedback

* fix: environment branch policy failing to find the created resource

The `Read` operation of the Environments Branch Policy resource
was failing to find the newly created Branch policies, due to
wrongly encoded environment name. Which cause the provider to
be inconsistent.

This fix uses `url.PathEscape` instead of `url.QueryEscape`
since we are using path parameters with the Github API in
that case. Additionally 2 operations - `Read` and `Delete`
don't need to use it as they receive the environment name
already parsed and attempting to encode it again breaks the
name.

* Fix incorrect merge

---------

Co-authored-by: Massimiliano Donini <massimiliano.donini@gmail.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
o-sama added a commit to o-sama/terraform-provider-github that referenced this pull request Aug 19, 2023
@o-sama
t # This is a combination of 27 commits.
ae35967 
Add docs, update tests, small changes to rulesets

octoherd: delete pull_request_template.md

feat: add pull_request_template.md PR template

feat: add data source to get organization members' SAML/SCIM linked identities (integrations#1778)

* add `github_organization_external_identities` which returns a list of github organization members and their SAML linked identity

* add docs

* add more fields to external_identities

* docs

---------

Co-authored-by: Keegan Campbell <me@kfcampbell.com>

fix: Github Environments Policy feature causing the provider to produce inconsistent result (integrations#1799)

* Add deployment policy resource

- Add the initial code to manage the resource
- Add sample configuration used to test it

TODO
- Documentation
- Tests

* Add schema description

* Fix creation of resource ID

* Add tests

* Add documentation

* Add terraform import support

* Undo example add

* Fix formatting

* PR feedback

* fix: environment branch policy failing to find the created resource

The `Read` operation of the Environments Branch Policy resource
was failing to find the newly created Branch policies, due to
wrongly encoded environment name. Which cause the provider to
be inconsistent.

This fix uses `url.PathEscape` instead of `url.QueryEscape`
since we are using path parameters with the Github API in
that case. Additionally 2 operations - `Read` and `Delete`
don't need to use it as they receive the environment name
already parsed and attempting to encode it again breaks the
name.

* Fix incorrect merge

---------

Co-authored-by: Massimiliano Donini <massimiliano.donini@gmail.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>

Enforce valid chars in repo name (integrations#1806)

* Enforce valid chars in repo name

Signed-off-by: Benoit Donneaux <ben@tergology.com>

* Add dash to validate repo name

Signed-off-by: Benoit Donneaux <ben@tergology.com>

* Better name validation message

Signed-off-by: Benoit Donneaux <ben@tergology.com>

* Test repo name max length

Signed-off-by: Benoit Donneaux <ben@tergology.com>

* Test space in repo name

Signed-off-by: Benoit Donneaux <ben@tergology.com>

---------

Signed-off-by: Benoit Donneaux <ben@tergology.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>

Use graphql rather than rest API for Github team membership (integrations#1786)

This change should provide better diffing for team membership for teams
with child teams. Currently, child team members are included in the parent
team's members list and so diffs are incorrect.

See [Issue 1193](integrations#1193).

feat: add immediate-response.yml auto responder workflow

build(deps): bump peter-evans/create-or-update-comment

Bumps [peter-evans/create-or-update-comment](https://github.com/peter-evans/create-or-update-comment) from 6534843181fc2aeb7f9f1cd3cd4a7b956cada2db to 716151b9579b05352dbf244d48e968d211889bbc.
- [Release notes](https://github.com/peter-evans/create-or-update-comment/releases)
- [Commits](peter-evans/create-or-update-comment@6534843...716151b)

---
updated-dependencies:
- dependency-name: peter-evans/create-or-update-comment
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Add Codespaces Organization Secret Repositories Resource.

Add depends_on to prevent branch policy being created before the corresponding environment (which results in an error), add more detail about the error thrown if deployment_branch_policy.custom_branch_policies is not set to true, tf fmt example

build(deps): bump peter-evans/create-or-update-comment (integrations#1830)

Bumps [peter-evans/create-or-update-comment](https://github.com/peter-evans/create-or-update-comment) from 716151b9579b05352dbf244d48e968d211889bbc to 38217c6b94b54c0dbbe75be237257364e2dd2e62.
- [Release notes](https://github.com/peter-evans/create-or-update-comment/releases)
- [Commits](peter-evans/create-or-update-comment@716151b...38217c6)

---
updated-dependencies:
- dependency-name: peter-evans/create-or-update-comment
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

build(deps): bump golang.org/x/crypto from 0.11.0 to 0.12.0 (integrations#1829)

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.11.0 to 0.12.0.
- [Commits](golang/crypto@v0.11.0...v0.12.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>

build(deps): bump golang.org/x/oauth2 from 0.10.0 to 0.11.0 (integrations#1828)

Bumps [golang.org/x/oauth2](https://github.com/golang/oauth2) from 0.10.0 to 0.11.0.
- [Commits](golang/oauth2@v0.10.0...v0.11.0)

---
updated-dependencies:
- dependency-name: golang.org/x/oauth2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

fix: `github_repository_collaborators` - mark `invitation_ids` as changed when new user invited (integrations#1825)

* fix: mark invitation_ids as changed if new user is invited

* fix: documentation

---------

Co-authored-by: Keegan Campbell <me@kfcampbell.com>

feat: expose SAML external identity exposed for GitHub user  (integrations#1796)

* create github_user_external_identity datasource

* add scim information and error handling for bad username org combo

* cleanup commentzs

* add docs for external identity

* move external identity to its own struct

* add variable to make referencing external identity easier

* add test

* add documentation

* remove old docs

* add docs reference in github.erb

---------

Co-authored-by: Keegan Campbell <me@kfcampbell.com>

Do not change squash_merge/merge_commit if it is not allowed in conf (integrations#1834)

Co-authored-by: Keegan Campbell <me@kfcampbell.com>

build(deps): bump peter-evans/create-or-update-comment

Bumps [peter-evans/create-or-update-comment](https://github.com/peter-evans/create-or-update-comment) from 38217c6b94b54c0dbbe75be237257364e2dd2e62 to 5f22cb87da9514ab329de42e5462372dc19928a5.
- [Release notes](https://github.com/peter-evans/create-or-update-comment/releases)
- [Commits](peter-evans/create-or-update-comment@38217c6...5f22cb8)

---
updated-dependencies:
- dependency-name: peter-evans/create-or-update-comment
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

add primary_language to repository datasource

add primary_language to repository resource

add test to repository datasource

add docs for primary_language change

add test for resource

update name of testcase

run linting

update comment

Update to go-github v54
@anoxape anoxape mentioned this pull request Sep 15, 2023
Merged
4 tasks
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
@edheliel @ilmax @kfcampbell
fix: Github Environments Policy feature causing the provider to produ…
ecf5d70 
…ce inconsistent result (integrations#1799)

* Add deployment policy resource

- Add the initial code to manage the resource
- Add sample configuration used to test it

TODO
- Documentation
- Tests

* Add schema description

* Fix creation of resource ID

* Add tests

* Add documentation

* Add terraform import support

* Undo example add

* Fix formatting

* PR feedback

* fix: environment branch policy failing to find the created resource

The `Read` operation of the Environments Branch Policy resource
was failing to find the newly created Branch policies, due to
wrongly encoded environment name. Which cause the provider to
be inconsistent.

This fix uses `url.PathEscape` instead of `url.QueryEscape`
since we are using path parameters with the Github API in
that case. Additionally 2 operations - `Read` and `Delete`
don't need to use it as they receive the environment name
already parsed and attempting to encode it again breaks the
name.

* Fix incorrect merge

---------

Co-authored-by: Massimiliano Donini <massimiliano.donini@gmail.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfcampbell kfcampbell kfcampbell 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.

Unable to set branch names/patterns when custom_branch_policies is true.
3 participants
@edheliel @kfcampbell @ilmax