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

feat: add web flag for codespace list & create subcommand #7288

Merged
merged 19 commits into from Apr 13, 2023

Conversation

doaortu
Copy link
Contributor

@doaortu doaortu commented Apr 7, 2023

This PR could fix #7261

Changes were made:


list subcommand

  • adding web flag for opening list of codespace in browser with url: https://github.com/codespaces.
  • --web flag can only be used with --repo flag, because in https://github.com/codespaces there only GET param for filtering with repository_id. (no params to filter with user or organizations). It will throws error if used with --user or org flag
  • when user only specify --web flag without additional flags, it will open browser with default url: https://github.com/codespaces
  • when user specify --web flag with --repo flag, it will first fetch repo_id from the api, and then open browser with url+repo_id param: https://github.com/codespaces?repository_id={id}
  • add test case this newly added --web flag with above scenario
  • modify the t.Run clause to add the &browser.Stub{} if tt.opts.useWeb is true (on pkg/cmd/codespace/list_test.go)

create subcommand

  • adding web flag for creating codespace through browser with url: https://github.com/codespaces
  • --web flag cannot be used with --display-name, --idle-timeout, or '--retention-period' flag because there's no option to set those in browser/Web UI. when used with those flags, it will throws error
  • when user only specify --web flag without additional flags, it will open browser with default url: https://github.com/codespaces/new
  • when user specify --web flag with --repo flag, it will first fetch repo_id from the api, and then open browser with url+repo_id param: https://github.com/codespaces/new?repo={id}&ref={defaultBranch}&machine={firstMachineInList}&location={emptyString}.
  • when no machine specified, it will automatically select the first machine instead of displaying machine chooser in terminal. (because already got machine chooser in web UI)
  • when no branch specified, it will automatically select the default branch for the repository
  • when no location specified, it will yield empty string to the location URL param, because the Web UI will automatically choose default location if none specified.
  • when additional flags specified (repo, branch, machine, location) , it will plug the value to the respective params: https://github.com/codespaces/new?repo={id}&ref={specifiedBranch}&machine={specifiedMachine}&location={specifiedLocation}
  • add test case this newly added --web flag with above scenario
  • modify the t.Run clause to add the &browser.Stub{} if tt.opts.useWeb is true (on pkg/cmd/codespace/create_test.go)

doaortu and others added 3 commits April 7, 2023 09:37
web flag only works with repo flag, because,
currently there only param for listing with repo_id
web flag used for creating codespace through web UI instead of terminal.
web flag cannot be used with display-name, idle-timeout,
or retention retention-period
because there's no option for that in the Web UI
@doaortu doaortu requested a review from a team as a code owner April 7, 2023 03:20
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Apr 7, 2023
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Apr 7, 2023
@jkeech
Copy link
Contributor

jkeech commented Apr 10, 2023

Thanks for opening this PR! Tagging @cmbrose to help review.

@doaortu
Copy link
Contributor Author

doaortu commented Apr 11, 2023

Thank you. Please let me know if there's anything I can help

Copy link
Contributor

@cmbrose cmbrose left a comment

Choose a reason for hiding this comment

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

Thanks for working this up and for updating the tests! It looks solid, just a few thoughts on extra things we might want to include here

pkg/cmd/codespace/create.go Show resolved Hide resolved
pkg/cmd/codespace/create.go Outdated Show resolved Hide resolved
pkg/cmd/codespace/create.go Outdated Show resolved Hide resolved
pkg/cmd/codespace/list.go Outdated Show resolved Hide resolved
pkg/cmd/codespace/list.go Outdated Show resolved Hide resolved
pkg/cmd/codespace/create.go Show resolved Hide resolved
doaortu and others added 5 commits April 12, 2023 12:14
- changed web flag mutual exclusive logic, using cmdutil
- extract that logic to PreRunE clause in createCmd
- move web flag up to make it close to PreRunE clause (for clarity)
- add new param to newCreateCmd fn to facilitate test logic
- apply new newCreateCmd fn to root.go
- remove 'yet' from error messages that can cause misunderstanding
- clarify list web flag can only be used with repo flag
(..and no machine flag provided)

+ add test for this new case
+ adjust related test cases for this new change.
why: err on PreRunE or RunE will also print help if error happened

+ move web, repo, org, user mutual exclusive logic to PreRunE clause
+ move repo, org, user mutual exclusive logic to PreRunE
+ move limit check flag to PreRunE
+ modify newListCmd fn to facilitate test logic
+ apply new newListCmd to root.go
+ add test cases to check PreRunE clause
- remove mutual exclusive test cases from Test_AppList
pkg/cmd/codespace/list.go Outdated Show resolved Hide resolved
pkg/cmd/codespace/list.go Outdated Show resolved Hide resolved
pkg/cmd/codespace/list.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cmbrose cmbrose left a comment

Choose a reason for hiding this comment

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

This is really coming together 🚀 Just a couple last style nits and we can merge this!

pkg/cmd/codespace/list.go Outdated Show resolved Hide resolved
pkg/cmd/codespace/create.go Outdated Show resolved Hide resolved
pkg/cmd/codespace/create_test.go Show resolved Hide resolved
pkg/cmd/codespace/list_test.go Show resolved Hide resolved
Copy link
Contributor

@cmbrose cmbrose left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for working through the iterations!

@doaortu
Copy link
Contributor Author

doaortu commented Apr 12, 2023

Thank you again for the detailed review, and for bearing with me until the end.

@cmbrose cmbrose merged commit 8332210 into cli:trunk Apr 13, 2023
6 checks passed
The GitHub CLI automation moved this from Needs review 🤔 to Pending Release 🥚 Apr 13, 2023
renovate bot added a commit to scottames/dots that referenced this pull request Apr 29, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [aquaproj/aqua-registry](https://togithub.com/aquaproj/aqua-registry)
| minor | `v3.158.0` -> `v3.159.0` |
| [cli/cli](https://togithub.com/cli/cli) | minor | `v2.27.0` ->
`v2.28.0` |
| [weaveworks/eksctl](https://togithub.com/weaveworks/eksctl) | minor |
`v0.138.0` -> `v0.139.0` |

---

### Release Notes

<details>
<summary>aquaproj/aqua-registry</summary>

###
[`v3.159.0`](https://togithub.com/aquaproj/aqua-registry/releases/tag/v3.159.0)

[Compare
Source](https://togithub.com/aquaproj/aqua-registry/compare/v3.158.1...v3.159.0)


[Issues](https://togithub.com/aquaproj/aqua-registry/issues?q=is%3Aissue+milestone%3Av3.159.0)
| [Pull
Requests](https://togithub.com/aquaproj/aqua-registry/pulls?q=is%3Apr+milestone%3Av3.159.0)
| aquaproj/aqua-registry@v3.158.0...v3.159.0

#### 🎉 New Packages


[#&#8203;11807](https://togithub.com/aquaproj/aqua-registry/issues/11807)
[kubecfg/kubecfg](https://togithub.com/kubecfg/kubecfg): A tool for
managing complex enterprise Kubernetes environments as code

[#&#8203;11808](https://togithub.com/aquaproj/aqua-registry/issues/11808)
[loov/goda](https://togithub.com/loov/goda): Go Dependency Analysis
toolkit

#### Fixes


[#&#8203;11806](https://togithub.com/aquaproj/aqua-registry/issues/11806)
solidiquis/erdtree: Follow up changes of erdtree v2.0.0

https://github.com/solidiquis/erdtree/releases/tag/v2.0.0

> Perhaps the most important change to note is that the compiled binary
has been renamed from et to erd in order to address the following issue
> regarding name collisions with other programs
>
> -
[solidiquis/erdtree#23

###
[`v3.158.1`](https://togithub.com/aquaproj/aqua-registry/releases/tag/v3.158.1)

[Compare
Source](https://togithub.com/aquaproj/aqua-registry/compare/v3.158.0...v3.158.1)


[Issues](https://togithub.com/aquaproj/aqua-registry/issues?q=is%3Aissue+milestone%3Av3.158.1)
| [Pull
Requests](https://togithub.com/aquaproj/aqua-registry/pulls?q=is%3Apr+milestone%3Av3.158.1)
| aquaproj/aqua-registry@v3.158.0...v3.158.1

#### Fixes


[#&#8203;11790](https://togithub.com/aquaproj/aqua-registry/issues/11790)
Follow up changes of cli/cli v2.28.0
[@&#8203;kyontan](https://togithub.com/kyontan)

GitHub's CLI (cli/cli) changed format for macOS to zip (from tar.gz)
since v2.28.0

See https://github.com/cli/cli/releases/tag/v2.28.0 for details.

</details>

<details>
<summary>cli/cli</summary>

### [`v2.28.0`](https://togithub.com/cli/cli/releases/tag/v2.28.0):
GitHub CLI 2.28.0

[Compare Source](https://togithub.com/cli/cli/compare/v2.27.0...v2.28.0)

#### What's New

-   macOS binaries are now signed and notarized
- ⚠️ macOS archives attached to our releases are no longer `.tar.gz`,
but `.zip` instead. This is because `.tar.gz` archives cannot be
notarized.
- The `checksums.txt` file attached to every release now includes the
checksum of the Windows MSI installer too.

- macOS and Windows binaries are now compiled from their respective
platforms and have `cgo` enabled. This might help resolve respecting
system proxy settings and avoid related networking issues.

- `issue edit`: edit multiple issues at the same time by
[@&#8203;heaths](https://togithub.com/heaths) in
[cli/cli#7259

- Add `gh org list` by
[@&#8203;joshkraft](https://togithub.com/joshkraft) in
[cli/cli#7257

- `ssh-key`: add ability to manage signing keys by
[@&#8203;kousikmitra](https://togithub.com/kousikmitra) in
[cli/cli#7270

- `search`: enable owner flag to take multiple values by
[@&#8203;kousikmitra](https://togithub.com/kousikmitra) in
[cli/cli#7305

- `codespace`: add `--web` flag for `list` & `create` commands by
[@&#8203;doaortu](https://togithub.com/doaortu) in
[cli/cli#7288

- Our Debian & RPM packages now ship with shell completion scripts by
[@&#8203;Xerkus](https://togithub.com/Xerkus) in
[cli/cli#7293

- `run list`: add `--event` and `--created` filters by
[@&#8203;cawfeecake](https://togithub.com/cawfeecake) in
[cli/cli#7363
[cli/cli#7352

- `repo`: add `visibility` JSON field by
[@&#8203;yeikel](https://togithub.com/yeikel) in
[cli/cli#7337

#### What's Changed

- Fix typo in `cs stop` command: `Stoppping` -> `Stopping` by
[@&#8203;FalseDev](https://togithub.com/FalseDev) in
[cli/cli#7318
- Update go-gh to v2 by [@&#8203;samcoe](https://togithub.com/samcoe) in
[cli/cli#7299
- `auth login`: normalize host name by
[@&#8203;tuananhlai](https://togithub.com/tuananhlai) in
[cli/cli#6999
- build(deps): bump github.com/cenkalti/backoff/v4 from 4.2.0 to 4.2.1
by [@&#8203;dependabot](https://togithub.com/dependabot) in
[cli/cli#7323
- Clarify how SSH keys are selected for `gh codespace ssh` by
[@&#8203;jkeech](https://togithub.com/jkeech) in
[cli/cli#7325
- Initialize deployment.yml workflow file by
[@&#8203;mislav](https://togithub.com/mislav) in
[cli/cli#7328
- Fix `gh cs ports` requiring `sudo` for privileged port numbers by
[@&#8203;cmbrose](https://togithub.com/cmbrose) in
[cli/cli#7326
- Correct some typos by [@&#8203;goggle](https://togithub.com/goggle) in
[cli/cli#7342
- Diacritics substitution in prompt by
[@&#8203;benjlevesque](https://togithub.com/benjlevesque) in
[cli/cli#7205
- gh: move `CODEOWNERS` inside the `.github/` dir by
[@&#8203;SauravMaheshkar](https://togithub.com/SauravMaheshkar) in
[cli/cli#7366
- Pretty-print gh api output when using --jq by
[@&#8203;mjpieters](https://togithub.com/mjpieters) in
[cli/cli#7236

#### New Contributors

- [@&#8203;joshkraft](https://togithub.com/joshkraft) made their first
contribution in
[cli/cli#7257
- [@&#8203;kousikmitra](https://togithub.com/kousikmitra) made their
first contribution in
[cli/cli#7270
- [@&#8203;doaortu](https://togithub.com/doaortu) made their first
contribution in
[cli/cli#7288
- [@&#8203;FalseDev](https://togithub.com/FalseDev) made their first
contribution in
[cli/cli#7318
- [@&#8203;tuananhlai](https://togithub.com/tuananhlai) made their first
contribution in
[cli/cli#6999
- [@&#8203;goggle](https://togithub.com/goggle) made their first
contribution in
[cli/cli#7342
- [@&#8203;Xerkus](https://togithub.com/Xerkus) made their first
contribution in
[cli/cli#7293
- [@&#8203;cawfeecake](https://togithub.com/cawfeecake) made their first
contribution in
[cli/cli#7363
- [@&#8203;yeikel](https://togithub.com/yeikel) made their first
contribution in
[cli/cli#7337
- [@&#8203;SauravMaheshkar](https://togithub.com/SauravMaheshkar) made
their first contribution in
[cli/cli#7366
- [@&#8203;mjpieters](https://togithub.com/mjpieters) made their first
contribution in
[cli/cli#7236

**Full Changelog**: cli/cli@v2.27.0...v2.28.0

</details>

<details>
<summary>weaveworks/eksctl</summary>

###
[`v0.139.0`](https://togithub.com/weaveworks/eksctl/releases/tag/v0.139.0):
eksctl 0.139.0 (permalink)

[Compare
Source](https://togithub.com/weaveworks/eksctl/compare/0.138.0...0.139.0)

### Release v0.139.0

#### 🚀 Features

- Security Policy for eksctl project
([#&#8203;6541](https://togithub.com/weaveworks/eksctl/issues/6541))

#### 🐛 Bug Fixes

- Fix flux version validation
([#&#8203;6530](https://togithub.com/weaveworks/eksctl/issues/6530))

#### 📝 Documentation

- Fix empty info block on Default Addon Upgrades page
([#&#8203;6524](https://togithub.com/weaveworks/eksctl/issues/6524))
- Update installation instructions
([#&#8203;6376](https://togithub.com/weaveworks/eksctl/issues/6376))
- AWS Private link support for fully private cluster
([#&#8203;6408](https://togithub.com/weaveworks/eksctl/issues/6408))

#### Acknowledgments

Weaveworks would like to sincerely thank:
[@&#8203;thezanke](https://togithub.com/thezanke), and
[@&#8203;yuxiang-zhang](https://togithub.com/yuxiang-zhang)

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

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

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/scottames/dots).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS41OC4yIiwidXBkYXRlZEluVmVyIjoiMzUuNjMuMSJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
jtpetty pushed a commit that referenced this pull request May 22, 2023
* feat: add web flag to cs list subcommand

web flag only works with repo flag, because,
currently there only param for listing with repo_id

* feat: add web flag to cs crate subcommand

web flag used for creating codespace through web UI instead of terminal.
web flag cannot be used with display-name, idle-timeout,
or retention retention-period
because there's no option for that in the Web UI

* refactor: extract mutual excusive logic to PreRunE

- changed web flag mutual exclusive logic, using cmdutil
- extract that logic to PreRunE clause in createCmd
- move web flag up to make it close to PreRunE clause (for clarity)
- add new param to newCreateCmd fn to facilitate test logic
- apply new newCreateCmd fn to root.go

* fix: clarify flag desc and error message

- remove 'yet' from error messages that can cause misunderstanding
- clarify list web flag can only be used with repo flag

* fix: skip machine check when we use web flag ...
(..and no machine flag provided)

+ add test for this new case
+ adjust related test cases for this new change.

* refactor: move flag check logic to PreRunE

why: err on PreRunE or RunE will also print help if error happened

+ move web, repo, org, user mutual exclusive logic to PreRunE clause
+ move repo, org, user mutual exclusive logic to PreRunE
+ move limit check flag to PreRunE
+ modify newListCmd fn to facilitate test logic
+ apply new newListCmd to root.go
+ add test cases to check PreRunE clause
- remove mutual exclusive test cases from Test_AppList

* refactor: remove the opts equality checks

* fix: mutually exclusive misfires
because of wrong logic

+ refine test case too

* cleanup:removing useWeb check in fn getMachineName

because it's no longer needed

+ remove redundant test-case

* refactor: remove redundant ifs

* refactor: clarify test name

* re-clarify web flag desc in list.go

* refactor: break long lines, use more idiomatic err

* add test case for nonexistent/wrong machine
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
No open projects
The GitHub CLI
  
Pending Release 🥚
Development

Successfully merging this pull request may close these issues.

Add support for "--web" to "gh cs {list,create}"
4 participants