Skip to content

Comments

APP-4896: Implement GetAuthApplication API#506

Merged
ohEmily merged 10 commits intoviamrobotics:mainfrom
jr22:APP-4896
May 22, 2024
Merged

APP-4896: Implement GetAuthApplication API#506
ohEmily merged 10 commits intoviamrobotics:mainfrom
jr22:APP-4896

Conversation

@jr22
Copy link
Contributor

@jr22 jr22 commented May 21, 2024

APP-4896

Implementing GetAuthApplication API that will eventually result in CLI command with format:

viam auth-app get --org-id=c6215428-1b73-41c3-b44a-56db0631c8f1 --application-id=6847e7a9-d7bd-4603-9655-bdc24370342a 
{
	"application_id": "6847e7a9-d7bd-4603-9655-bdc24370342a",
	"application_name": "julias app",
	"secret": "supersupersecretsecret"
	"origin_uris": ["https://test.com", "https://test2.com" ],
	"redirect_uris": ["https://redirect.com", "https://redirect2.com" ],
	"logout_uri": "https://logout.com",
}

@github-actions github-actions bot added the safe to test committer is a member of this org label May 21, 2024
@jr22 jr22 requested a review from ohEmily May 21, 2024 21:33
message GetAuthApplicationResponse {
string application_id = 1;
string application_name = 2;
string secret = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] maybe we rename to client_secret to match FusionAuth's application.oauthConfiguration.clientSecret from their API? also makes it a little easier to grep for this string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah makes sense to me. i can also rename the secret field in the RegisterAuthApplication request here to client_secret then?

Copy link
Contributor

@ohEmily ohEmily 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! Added a small comment.

@jr22
Copy link
Contributor Author

jr22 commented May 22, 2024

workflow is broken for api changes -> fixing here

@ohEmily
Copy link
Contributor

ohEmily commented May 22, 2024

Weird that changing this broke the linter suite. Seems like a linter bug. To unblock your PR you could squash the changes.

/ Test For Lint and Breaking Changes
Field "3" with name "client_secret" on message "RegisterAuthApplicationResponse" changed option "json_name" from "secret" to "clientSecret".
 Check failure on line 47 in proto/viam/app/v1/end_user.proto

/ Test For Lint and Breaking Changes
Field "3" on message "RegisterAuthApplicationResponse" changed name from "secret" to "client_secret".

@jr22
Copy link
Contributor Author

jr22 commented May 22, 2024

Weird that changing this broke the linter suite. Seems like a linter bug. To unblock your PR you could squash the changes.

/ Test For Lint and Breaking Changes
Field "3" with name "client_secret" on message "RegisterAuthApplicationResponse" changed option "json_name" from "secret" to "clientSecret".
 Check failure on line 47 in proto/viam/app/v1/end_user.proto

/ Test For Lint and Breaking Changes
Field "3" on message "RegisterAuthApplicationResponse" changed name from "secret" to "client_secret".

Its a breaking change for me to change the RegisterAuthResponse field from secret -> client_secret so i think this failure is expected?

@jr22 jr22 added ready-for-protos add this when you want protos to compile on every commit and removed ready-for-protos add this when you want protos to compile on every commit labels May 22, 2024
@jr22 jr22 added ready-for-protos add this when you want protos to compile on every commit and removed ready-for-protos add this when you want protos to compile on every commit labels May 22, 2024
@jr22 jr22 added the ready-for-protos add this when you want protos to compile on every commit label May 22, 2024
@jr22 jr22 removed the ready-for-protos add this when you want protos to compile on every commit label May 22, 2024
@jr22 jr22 added the ready-for-protos add this when you want protos to compile on every commit label May 22, 2024
@jr22 jr22 removed the ready-for-protos add this when you want protos to compile on every commit label May 22, 2024
@ohEmily ohEmily merged commit 163235b into viamrobotics:main May 22, 2024
@ohEmily
Copy link
Contributor

ohEmily commented May 22, 2024

Weird that changing this broke the linter suite. Seems like a linter bug. To unblock your PR you could squash the changes.

/ Test For Lint and Breaking Changes
Field "3" with name "client_secret" on message "RegisterAuthApplicationResponse" changed option "json_name" from "secret" to "clientSecret".
 Check failure on line 47 in proto/viam/app/v1/end_user.proto

/ Test For Lint and Breaking Changes
Field "3" on message "RegisterAuthApplicationResponse" changed name from "secret" to "client_secret".

Its a breaking change for me to change the RegisterAuthResponse field from secret -> client_secret so i think this failure is expected?

Oh what I meant is that it seems dumb to consider it a "breaking" change when it's comparing to an earlier version of the same PR. IMO it should only compare to main, not to the current branch.

@jr22 jr22 deleted the APP-4896 branch May 22, 2024 19:29
@jr22
Copy link
Contributor Author

jr22 commented May 22, 2024

Weird that changing this broke the linter suite. Seems like a linter bug. To unblock your PR you could squash the changes.

/ Test For Lint and Breaking Changes
Field "3" with name "client_secret" on message "RegisterAuthApplicationResponse" changed option "json_name" from "secret" to "clientSecret".
 Check failure on line 47 in proto/viam/app/v1/end_user.proto

/ Test For Lint and Breaking Changes
Field "3" on message "RegisterAuthApplicationResponse" changed name from "secret" to "client_secret".

Its a breaking change for me to change the RegisterAuthResponse field from secret -> client_secret so i think this failure is expected?

Oh what I meant is that it seems dumb to consider it a "breaking" change when it's comparing to an earlier version of the same PR. IMO it should only compare to main, not to the current branch.

It is comparing to main though. On main the "RegisterAuthApplicationResponse" has "secret" not "client_secret". Maybe im not understanding your q?

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

Labels

protos-compiled safe to test committer is a member of this org

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants