Skip to content

test: Create BYOID Integration tests #719

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
Merged
busunkim96 merged 17 commits into from
Mar 26, 2021
Merged

test: Create BYOID Integration tests #719

busunkim96 merged 17 commits into from
Mar 26, 2021

Conversation

@ScruffyProdigy
Copy link
Contributor

@ScruffyProdigy ScruffyProdigy commented Mar 11, 2021

We are adding in regression tests for BYOID. This PR focuses on
file-based BYOID

@ScruffyProdigy ScruffyProdigy requested a review from a team as a code owner March 11, 2021 14:33
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 11, 2021
test: Create BYOID Integration tests
7d5d434 
We are adding in regression tests for BYOID.  This PR focuses on
file-based BYOID
@bojeil-google
Copy link
Contributor

bojeil-google commented Mar 11, 2021 •
edited
Loading

@bojeil-google to take a look

@ScruffyProdigy ScruffyProdigy changed the title Create BYOID Integration tests test: Create BYOID Integration tests Mar 11, 2021
@busunkim96 busunkim96 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 12, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 13, 2021
busunkim96
busunkim96 reviewed Mar 13, 2021
View reviewed changes
bojeil-google
bojeil-google suggested changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@bojeil-google bojeil-google 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 putting this together and I apologize for the delay.
Will take another look tomorrow.

@bojeil-google
Copy link
Contributor

bojeil-google commented Mar 23, 2021

Hey @ScruffyProdigy you missed some of the requested changes for the scripts/setup_external_accounts.sh script. Let me know when that's done and we can continue the review.

@ScruffyProdigy
Copy link
Contributor Author

ScruffyProdigy commented Mar 23, 2021

@bojeil-google
I added the comments you requested. I can make the changes to the bash code if you really want them, but if so, we should think about updating the script in all of the client libraries that use it to keep them consistent

bojeil-google
bojeil-google approved these changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

I think we can leave the script for now. Most importantly we added the explanation on what the script does. The other changes I requested were mostly improvements.
I have a few more minor stuff. Otherwise, the change looks good from my end. I'll leave it to @busunkim96 to finish the review from her end.

busunkim96
busunkim96 suggested changes Mar 25, 2021
View reviewed changes
@busunkim96 busunkim96 merged commit 48e8be3 into googleapis:master Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@busunkim96 busunkim96 busunkim96 approved these changes

@bojeil-google bojeil-google bojeil-google approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ScruffyProdigy @bojeil-google @busunkim96 @yoshi-kokoro