Skip to content

Add list options support for environments#2258

Merged
gmlewis merged 6 commits into
google:masterfrom
chelnak:add_list_options_support_for_environments
Feb 3, 2022
Merged

Add list options support for environments#2258
gmlewis merged 6 commits into
google:masterfrom
chelnak:add_list_options_support_for_environments

Conversation

@chelnak

@chelnak chelnak commented Jan 17, 2022

Copy link
Copy Markdown
Contributor

Closes #2257

This PR introduces an EnvironmentListOptions parameter for the ListEnvironments method.

Tests have also been updated to reflect the changes and an example has been added to the example directory.

This commit updates existing tests for ListEnvironments to ensure that
they will be compatible with the EnvironmentListOptions implementation.
This commit implements EnvironmentListOptions for the ListEnvironments
method by adding a new parameter and using the addOptions function.

Additionally I've created a new struct called EnvironmentListOptions
and embedded ListOptions with the idea that this could be expanded
one day.
This commit adds an example to show how to work with ListEnvironments
and it's new opts parameter.
@gmlewis gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Jan 17, 2022

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @chelnak !
Just a few changes are needed for the example... then we will be ready for a second LGTM before merging.

Comment thread example/listenvironments/main.go Outdated
Comment thread example/listenvironments/main.go Outdated
Comment thread example/listenvironments/main.go Outdated
Comment thread example/listenvironments/main.go Outdated
Comment thread example/listenvironments/main.go
Comment thread example/listenvironments/main.go Outdated
@gmlewis gmlewis requested a review from wesleimp January 17, 2022 15:29
@codecov

codecov Bot commented Jan 17, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2258 (ee239cd) into master (2950205) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2258   +/-   ##
=======================================
  Coverage   97.81%   97.81%           
=======================================
  Files         114      114           
  Lines       10266    10269    +3     
=======================================
+ Hits        10042    10045    +3     
  Misses        156      156           
  Partials       68       68           
Impacted Files Coverage Δ
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 2950205...ee239cd. Read the comment docs.

Made some minor change to this example that include removing os.Exit(),
fixing some unnecessary variable declarations and godoc improvements.
@chelnak

chelnak commented Jan 17, 2022

Copy link
Copy Markdown
Contributor Author

@gmlewis All of your comments should now be resolved. Thanks for taking the time to review.

"unnecessary leading newline (whitespace)"
Comment thread example/listenvironments/main.go Outdated
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @chelnak !
LGTM.
Awaiting second LGTM before merging.

@chelnak

chelnak commented Jan 24, 2022

Copy link
Copy Markdown
Contributor Author

Hello! Is there anything else you would like me to change for this PR?

@gmlewis

gmlewis commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

Hello! Is there anything else you would like me to change for this PR?

No, thank you. We are awaiting any other contributor to this repo to give an LGTM and approve this PR before merging.

@chelnak

chelnak commented Feb 2, 2022

Copy link
Copy Markdown
Contributor Author

Hey hey! Hope everything is going well! Wondering if we could request a review from another contributor 🙏? Appreciate everyone is super busy though.

@gmlewis

gmlewis commented Feb 3, 2022

Copy link
Copy Markdown
Collaborator

Sorry for the delay, @chelnak !
Hopefully by tomorrow we will be able to get this one merged. Maybe @Parker77 has time to review?

@chelnak

chelnak commented Feb 3, 2022

Copy link
Copy Markdown
Contributor Author

Thank you 🙏 😄

@Parker77 Parker77 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM.

@gmlewis

gmlewis commented Feb 3, 2022

Copy link
Copy Markdown
Collaborator

Than you, @Parker77 !
Merging.

@gmlewis gmlewis merged commit bda32dc into google:master Feb 3, 2022
@fun4cash

fun4cash commented Apr 7, 2022

Copy link
Copy Markdown

import "github.com/google/go-github/v43/github"

jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support pagination for ListEnvironments

4 participants