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.

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

chore: Add SecretManager service regional code samples #2880

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

chore: Add SecretManager service regional code samples #2880

wants to merge 6 commits into from

Conversation

YashSaraf11
Copy link
Contributor

Changes

  • Added code samples for regional APIs.
  • Added test files for the regional code samples added.
  • Added RegionalSecretManagerFixture.cs file for testing the regional code samples added.

@YashSaraf11 YashSaraf11 requested review from a team as code owners December 23, 2024 09:28
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Dec 23, 2024
@snippet-bot Snippet Bot
Copy link

snippet-bot bot commented Dec 23, 2024 •
edited
Loading

Here is the summary of changes.

You are about to add 28 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@amanda-tarafa amanda-tarafa assigned amanda-tarafa and unassigned jskeet Jan 6, 2025
@amanda-tarafa amanda-tarafa added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 6, 2025
@YashSaraf11 YashSaraf11 requested a review from vipul7499 January 7, 2025 10:07
amanda-tarafa
amanda-tarafa requested changes Jan 28, 2025
View reviewed changes
Copy link
Member

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

Please rebase this PR on main as at the moment your branch is outdated and this PR is including unrelated Secret Manager changes.

There are several comments that although left in one or two files only, should be addressed everywhere.

I aslo have some concerns about:

  • assumptions that tests run in a given order
  • assumptions that tests do not run in parallel
  • reuse of fxiture resources in tests that modify said resources

All of these may be addressed by creating specific resources for each of the tests that modify the resources. And ideally those should be set up and deleted on each of the tests using the fixture methods as helpers. Shares resources in the fixture should only be used for tests that do not modify those resources. I know some of this was inspired by the existing global samples, but, although those are not perfect, there are way less of them that are modifying resources, and certainly not relying on the eTag for optimistic concurrent updates. This PR as is will be flaky.

Also, the code doesn't currently build, there's a type in a variable name.

@YashSaraf11
Copy link
Contributor Author

Please rebase this PR on main as at the moment your branch is outdated and this PR is including unrelated Secret Manager changes.

There are several comments that although left in one or two files only, should be addressed everywhere.

I aslo have some concerns about:

  • assumptions that tests run in a given order
  • assumptions that tests do not run in parallel
  • reuse of fxiture resources in tests that modify said resources

All of these may be addressed by creating specific resources for each of the tests that modify the resources. And ideally those should be set up and deleted on each of the tests using the fixture methods as helpers. Shares resources in the fixture should only be used for tests that do not modify those resources. I know some of this was inspired by the existing global samples, but, although those are not perfect, there are way less of them that are modifying resources, and certainly not relying on the eTag for optimistic concurrent updates. This PR as is will be flaky.

Also, the code doesn't currently build, there's a type in a variable name.

Have made the changes in the file as per the suggestion, thanks for pointing it out.

@YashSaraf11 YashSaraf11 requested review from a team and chenlei1216 as code owners February 12, 2025 09:53
vipul7499
vipul7499 previously approved these changes Feb 13, 2025
View reviewed changes
@amanda-tarafa amanda-tarafa removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 18, 2025
amanda-tarafa
amanda-tarafa requested changes Feb 18, 2025
View reviewed changes
Copy link
Member

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

@YashSaraf11 Can you rebase this PR on main? It now has accumulated changes for other products with changes in over 160 files, and it's very hard to review. Thanks!

@YashSaraf11
Copy link
Contributor Author

@YashSaraf11 Can you rebase this PR on main? It now has accumulated changes for other products with changes in over 160 files, and it's very hard to review. Thanks!

Have rebased this PR on main. Sorry for the inconvenience.

amanda-tarafa
amanda-tarafa requested changes Feb 21, 2025
View reviewed changes
Copy link
Member

@amanda-tarafa amanda-tarafa 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 looking good in terms of possible flakiness because of resource sharing across tests. I think there a couple of tests that still need updating, I've called those out. Thanks!

There are a couple other comments for sample code simplification.

And there are the style comments, about the new-line parenthesis and indents. Those are off. I replied to your comment about dotnet format. We should fix these at least in sample code, it happens in some tests as well, because it's really an unfamiliar style.

secretmanager/api/SecretManager.Samples.Tests/IamRegionalRevokeAccessTests.cs Outdated
[Fact]
public void GrantsAccess()
{
SecretName secretName = _fixture.SecretToCreateName;
Copy link
Member

Choose a reason for hiding this comment

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

I think you forgot to change this one.

@YashSaraf11 YashSaraf11 force-pushed the add-regional-samples branch from a66a598 to 8ee91ca Compare March 11, 2025 14:13
@YashSaraf11 YashSaraf11 force-pushed the add-regional-samples branch from 8ee91ca to 56c7795 Compare March 11, 2025 14:28
YashSaraf11
YashSaraf11 commented Mar 11, 2025
View reviewed changes
Copy link
Contributor Author

@YashSaraf11 YashSaraf11 left a comment

Choose a reason for hiding this comment

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

@amanda-tarafa
Thanks for the detailed review! I've incorporated your feedback and fixed the dotnet format issues.
I also looked into the secret updates. As you mentioned, they're updating different pieces and don't affect the tests.

I believe I've addressed all your points, but please let me know if anything else comes up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amanda-tarafa amanda-tarafa amanda-tarafa requested changes

@vipul7499 vipul7499 vipul7499 left review comments

@meteatamel meteatamel Awaiting requested review from meteatamel

@chenlei1216 chenlei1216 Awaiting requested review from chenlei1216

Requested changes must be addressed to merge this pull request.

Assignees

@amanda-tarafa amanda-tarafa

Labels
samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@YashSaraf11 @amanda-tarafa @vipul7499 @jskeet