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

Refactor IAM-related classes to be ready to support more services #1224

Merged
merged 3 commits into from
Sep 6, 2016

Conversation

mziccard
Copy link
Contributor

@mziccard mziccard commented Sep 2, 2016

This PR does the following:

  • Remove resourcemanager's Policy class
  • Rename core's IamPolicy to Policy
  • Add Role class to hold string values for roles
  • Add Policy.Marshaller and Policy.DefaultMarshaller classes to convert Policy to/from gRPC protos
  • Add PolicyMarshaller to resourcemanager to convert Policy to/from resourcemanager's protos
  • Update READMEs, javadoc and examples

@aozarov @ajkannan do you mind having a look? Also, should we add an iam package?

- Remove resourcemanager's Policy class
- Rename core's IamPolicy to Policy
- Add Role class to hold string values for roles
- Add Policy.Marshaller and Policy.DefaultMarshaller classes to convert Policy to/from gRPC protos
- Add PolicyMarshaller to resourcemanager to convert Policy to/from resourcemanager's protos
- Update READMEs, javadoc and examples
@mziccard mziccard added api: core api: cloudresourcemanager Issues related to the Resource Manager API. labels Sep 2, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 2, 2016
@@ -37,69 +44,127 @@
* a set of identities to a role, where the identities can be user accounts, Google groups, Google

This comment was marked as spam.

This comment was marked as spam.

@aozarov
Copy link
Contributor

aozarov commented Sep 2, 2016

Looks good (only skimmed the tests though). Few comments/suggestions.

I am not convinced that we should put the Policy under iam sub-package unless we expect to have more kinds of policies in the future but fine either way.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 85.207% when pulling 1431f0d on mziccard:refactor-iam into 7309be5 on GoogleCloudPlatform:master.

@ajkannan
Copy link

ajkannan commented Sep 2, 2016

LGTM

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 85.209% when pulling 462910e on mziccard:refactor-iam into 3ea98b1 on GoogleCloudPlatform:master.

@mziccard
Copy link
Contributor Author

mziccard commented Sep 5, 2016

Comments fixed, PTAL

I am not convinced that we should put the Policy under iam sub-package unless we expect to have more kinds of policies in the future but fine either way.

I was considering to put: Policy, Role and Identity under the iam package but I am not convinced either.

@aozarov
Copy link
Contributor

aozarov commented Sep 6, 2016

LGTM (one comment was not addressed, your call).

@mziccard
Copy link
Contributor Author

mziccard commented Sep 6, 2016

@aozarov Another thing we might want to discuss is wether to store etag as byte[] or String. gRPC stores it as a ByteString (immutable byte array), we currently store it as String, do you have any preferences?

@aozarov
Copy link
Contributor

aozarov commented Sep 6, 2016

My preference would be that as long as we wrap (and not expose) the underlying protobuf we should return it as String (which is also what we did for the api-client based Services).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 85.209% when pulling c046d6e on mziccard:refactor-iam into 3ea98b1 on GoogleCloudPlatform:master.

@aozarov aozarov merged commit a493457 into googleapis:master Sep 6, 2016
github-actions bot pushed a commit that referenced this pull request Aug 25, 2022
…#1224)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:google-cloud-core](https://togithub.com/googleapis/java-core) | `2.8.9` -> `2.8.10` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-core/2.8.10/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-core/2.8.10/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-core/2.8.10/compatibility-slim/2.8.9)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-core/2.8.10/confidence-slim/2.8.9)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

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

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost.

---

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/googleapis/java-automl).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNzQuMSIsInVwZGF0ZWRJblZlciI6IjMyLjE3NC4xIn0=-->
github-actions bot pushed a commit that referenced this pull request Aug 25, 2022
🤖 I have created a release *beep* *boop*
---


## [2.3.6](googleapis/java-automl@v2.3.5...v2.3.6) (2022-08-25)


### Dependencies

* update dependency com.google.cloud:google-cloud-core to v2.8.10 ([#1224](googleapis/java-automl#1224)) ([731ed9f](googleapis/java-automl@731ed9f))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@release-please release-please bot mentioned this pull request Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: cloudresourcemanager Issues related to the Resource Manager API. api: core cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants