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

Add support for environments and environment secrets #1847

Merged
merged 9 commits into from
Apr 8, 2021
Merged

Add support for environments and environment secrets #1847

merged 9 commits into from
Apr 8, 2021

Conversation

srgustafson8
Copy link
Contributor

Closes #1843

@google-cla google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Apr 4, 2021
@codecov
Copy link

codecov bot commented Apr 4, 2021

Codecov Report

Merging #1847 (698b496) into master (bf34728) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1847      +/-   ##
==========================================
+ Coverage   97.59%   97.63%   +0.03%     
==========================================
  Files         104      105       +1     
  Lines        6615     6716     +101     
==========================================
+ Hits         6456     6557     +101     
  Misses         86       86              
  Partials       73       73              
Impacted Files Coverage Δ
github/actions_secrets.go 100.00% <100.00%> (ø)
github/repos_environments.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf34728...698b496. Read the comment docs.

Copy link
Collaborator

@gmlewis gmlewis 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, @srgustafson8 !

github/actions_secrets.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments_test.go Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
srgustafson8 and others added 2 commits April 4, 2021 23:00
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Copy link
Collaborator

@gmlewis gmlewis 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, @srgustafson8!
Just a few more tweaks, please, and then we should be ready for a second LGTM before merging.

github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
github/repos_environments.go Outdated Show resolved Hide resolved
@gmlewis gmlewis requested a review from wesleimp April 6, 2021 13:48
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@srgustafson8
Copy link
Contributor Author

Thank you, @srgustafson8!
Just a few more tweaks, please, and then we should be ready for a second LGTM before merging.

Thank you for the reviews @gmlewis - those should now be resolved!

Copy link
Collaborator

@gmlewis gmlewis 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, @srgustafson8 !
LGTM.

Awaiting second LGTM before merging.

Copy link
Collaborator

@wesleimp wesleimp left a comment

Choose a reason for hiding this comment

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

💙

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 8, 2021

Thank you, @wesleimp !
Merging.

Comment on lines +16 to +21
Owner *string `json:"owner,omitempty"`
Repo *string `json:"repo,omitempty"`
EnvironmentName *string `json:"environment_name,omitempty"`
WaitTimer *int `json:"wait_timer,omitempty"`
Reviewers []*EnvReviewers `json:"reviewers,omitempty"`
DeploymentBranchPolicy *BranchPolicy `json:"deployment_branch_policy,omitempty"`
Copy link
Contributor

@jvm986 jvm986 Mar 30, 2023

Choose a reason for hiding this comment

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

I wonder if I can make a comment here for some clarification on what these fields actually mean. The fields below cover the response from the API for this object, but afaict these highlighted fields are always nil.

I am sure I am missing something, so if possible could someone explain what they are for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The official docs say this:

Note: To get information about name patterns that branches must match in order to deploy to this environment, see "Get a deployment branch policy."

Does that help?
Do you want to create a PR explaining your understanding of the official docs to this repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @jvm986 - if you're referring to the Return/response only values (lines 22-30) I believe I put them in as they featured in the response schema when getting an environment. Are you seeing them null/nil in both Get & Create operations?

Copy link
Contributor

@jvm986 jvm986 Apr 1, 2023

Choose a reason for hiding this comment

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

Hi guys, thanks for the response!

I am referring to the highlighted vales (Owner Repo EnvironmentName WaitTimer Reviewers DeploymentBranchPolicy). I am seeing only nil values in GET operations (this struct is only used in GET operations at this point --> ListEnvironments() & GetEnvironment(), we are using the CreateUpdateEnvironment struct for PUT operations with CreateUpdateEnvironment()) and these values do not appear in the docs.

Again, I might be missing something. But for example the WaitTimer value I am currently parsing from the ProtectionRules slice in my code as it's nil in the struct:
Screenshot 2023-04-01 at 10 03 52

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 1, 2023

In cases like these, where you are expecting to get a value back from the server but it is missing, one of the best ways to debug is to add something like https://github.com/gmlewis/go-httpdebug to your code and make the call again to see the curl-equivalent of the call you are making. Then try to make the same call actually using curl and see if you get different values.

If curl gives you the same results as this repo, then the next step is to contact GitHub tech support and ask why you are not getting back the fields that you want.

If, however, curl gives you what you are looking for but this repo's client library does not, then it is time to debug why and open up a PR to fix the problem.

Good luck, and let us know if we can help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Environments and Environment Secrets
4 participants