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

Initial implementation of codespaces API #2803

Merged
merged 10 commits into from
Jun 19, 2023

Conversation

artificial-aidan
Copy link
Contributor

@google-cla
Copy link

google-cla bot commented Jun 9, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

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.

Nice job, @artificial-aidan!
Just a few changes please, and then we will be ready for a second LGTM+Approval before merging.

github/codespaces.go Outdated Show resolved Hide resolved
github/codespaces_test.go Outdated Show resolved Hide resolved
github/codespaces.go Outdated Show resolved Hide resolved
github/codespaces.go Outdated Show resolved Hide resolved
Comment on lines 70 to 71
StorageInBytes *int `json:"storage_in_bytes,omitempty"`
MemoryInBytes *int `json:"memory_in_bytes,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
StorageInBytes *int `json:"storage_in_bytes,omitempty"`
MemoryInBytes *int `json:"memory_in_bytes,omitempty"`
StorageInBytes *int64 `json:"storage_in_bytes,omitempty"`
MemoryInBytes *int64 `json:"memory_in_bytes,omitempty"`

github/codespaces.go Outdated Show resolved Hide resolved
github/codespaces.go Outdated Show resolved Hide resolved
github/codespaces.go Outdated Show resolved Hide resolved
github/codespaces.go Outdated Show resolved Hide resolved
github/codespaces.go Outdated Show resolved Hide resolved
@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jun 9, 2023
@artificial-aidan
Copy link
Contributor Author

Nice job, @artificial-aidan! Just a few changes please, and then we will be ready for a second LGTM+Approval before merging.

I'll push those up. I also renamed some of the list methods to match others in the repo.

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 9, 2023

Thank you!
Please run "go generate ./..." in the top of the repo and push (not force-push) the changes to the PR. (...and also run gofmt, of course.)
Also, please remember to sign the CLA.
See CONTRIBUTING.md for more details.

@artificial-aidan
Copy link
Contributor Author

Thank you! Please run "go generate ./..." in the top of the repo and push (not force-push) the changes to the PR. (...and also run gofmt, of course.) Also, please remember to sign the CLA. See CONTRIBUTING.md for more details.

Working on the CLA now.

Should I push the suggested changes, then run go generate?

Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #2803 (bbc2995) into master (3efdd2c) will decrease coverage by 0.06%.
The diff coverage is 95.47%.

@@            Coverage Diff             @@
##           master    #2803      +/-   ##
==========================================
- Coverage   98.06%   98.00%   -0.06%     
==========================================
  Files         132      134       +2     
  Lines       11650    11915     +265     
==========================================
+ Hits        11424    11677     +253     
- Misses        154      162       +8     
- Partials       72       76       +4     
Impacted Files Coverage Δ
github/messages.go 100.00% <ø> (ø)
github/codespaces.go 92.40% <92.40%> (ø)
github/codespaces_secrets.go 96.72% <96.72%> (ø)
github/event.go 100.00% <100.00%> (ø)
github/github.go 97.97% <100.00%> (+<0.01%) ⬆️

@artificial-aidan
Copy link
Contributor Author

The last commit basically mirrors the functionality and layout of Action secrets, but consolidated the tests a bit. I added some examples I used when validating its functionality against a real github.

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 9, 2023

Please don't use force-push in this repo, as we always squash-and-merge at the end anyway.
It makes reviews on GitHub particularly painful to see what changed since the last review.
See CONTRIBUTING.md for more details.

@artificial-aidan
Copy link
Contributor Author

Please don't use force-push in this repo, as we always squash-and-merge at the end anyway. It makes reviews on GitHub particularly painful to see what changed since the last review. See CONTRIBUTING.md for more details.

Sorry about that. Must have been out of habit 👍

example/codespaces/newreposecretwithxcrypto/main.go Outdated Show resolved Hide resolved
example/codespaces/newreposecretwithxcrypto/main.go Outdated Show resolved Hide resolved
example/codespaces/newusersecretwithxcrypto/main.go Outdated Show resolved Hide resolved
github/codespaces.go Outdated Show resolved Hide 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, @artificial-aidan !
LGTM.

Awaiting CLA and LGTM+Approval from any other contributor to this repo before merging.

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 9, 2023

It looks like you might need to run gofmt and go generate ./... again.

@artificial-aidan
Copy link
Contributor Author

@gmlewis got the CLA signed

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, @artificial-aidan !
LGTM.

Awaiting LGTM+Approval from any other contributor to this repo before merging.

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 19, 2023

Thank you, @valbeat !
Merging.

@gmlewis gmlewis merged commit 56c2c3b into google:master Jun 19, 2023
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jun 19, 2023
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.

3 participants