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

GoogleAuth#getProjectId vs. GoogleAuth#getDefaultProjectId #389

Closed
ofrobots opened this issue Jun 21, 2018 · 7 comments
Closed

GoogleAuth#getProjectId vs. GoogleAuth#getDefaultProjectId #389

ofrobots opened this issue Jun 21, 2018 · 7 comments
Assignees
Labels
semver: major Hint for users that this is an API breaking change. type: cleanup An internal cleanup or hygiene concern.

Comments

@ofrobots
Copy link
Contributor

These are synonyms of each other, but given then name, these should NOT be.

I would intuit that getDefaultProjectId always gives the default project Id as per the environment (GCLOUD_PROJECT, GOOGLE_APPLICATION_CREDENTIALS, fetch from metadata service, etc.) and never the configured one. As named, it is intuitively incorrect for it to return the configured projectId.

I don't see a reason why both of these names should exist, unless we want to support both behaviours.

IMO, getDefaultProjectId should be dropped. If we want to keep both names, they should do what their names say they do.

@ofrobots ofrobots changed the title GoogleAuth#getProjectId vs. GoogleAuth.getDefaultProjectId GoogleAuth#getProjectId vs. GoogleAuth#getDefaultProjectId Jun 21, 2018
@JustinBeckwith JustinBeckwith added the triage me I really want to be triaged. label Jun 21, 2018
@ofrobots
Copy link
Contributor Author

IOW, this test (#388) makes sense to pass for getProjectId, but not for getDefaultProjectId.

@ofrobots
Copy link
Contributor Author

@JustinBeckwith it seems like the behaviour of getDefaultProjectId was changed in #281. Before that it used to do what the name implied.

@JustinBeckwith JustinBeckwith added the type: cleanup An internal cleanup or hygiene concern. label Jun 21, 2018
@JustinBeckwith
Copy link
Contributor

Yikes. You're totally right. In the spirit of semver, what do you think makes the most sense here? I broke the behavior in a semver minor change. Would it be semver minor to change it back? Or does it make more sense to do a major rev?

@JustinBeckwith JustinBeckwith removed the triage me I really want to be triaged. label Jun 21, 2018
@ofrobots
Copy link
Contributor Author

IMO fixing the bug would be a semver-patch. Opinions might differ if the bug existed for a very long time and people expected the buggy behavior to be the actual behavior. That's not the case here.

@JustinBeckwith
Copy link
Contributor

Taking a closer look here - I don't think having a method that just gets the default projectId is actually valuable. Happy to hear scenarios if you think it is 🤷‍♂️

Given that, I prefer just deleting the getDefaultProjectId method all together. Thoughts?

@JustinBeckwith JustinBeckwith added semver: major Hint for users that this is an API breaking change. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jun 22, 2018
@ofrobots
Copy link
Contributor Author

👍 on removing getDefaultProjectId as suggested in OP.

@JustinBeckwith JustinBeckwith removed the priority: p2 Moderately-important priority. Fix may not be included in next release. label Jul 1, 2018
@JustinBeckwith
Copy link
Contributor

Fixed in #402

@JustinBeckwith JustinBeckwith self-assigned this Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major Hint for users that this is an API breaking change. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

2 participants