Skip to content

feat: update OPENAPI spec for Service Account#121

Merged
craicoverflow merged 8 commits intomasterfrom
credentials-api
Dec 7, 2020
Merged

feat: update OPENAPI spec for Service Account#121
craicoverflow merged 8 commits intomasterfrom
credentials-api

Conversation

@craicoverflow
Copy link
Contributor

@craicoverflow craicoverflow commented Dec 4, 2020

Closes #115

Based on discussions in #115 the command has been changed to a second-level service-account command group.

$ rhoas serviceaccount create ...

Tasks

  • Make output format --output a mandatory flag with no default. This is to prevent the user having to create more than one service account because they did not want .env which is the default
  • --output-file flag to let the user customise the location the credentials are written to.
  • Sanitize output file location
  • Allow ability to supply name and description

paths:
/api/managed-services-api/v1/serviceAccount:
get:
/api/managed-services-api/v1/serviceaccounts:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have manually added this, because the MAS API does not have it yet.

@wtrocki
Copy link
Collaborator

wtrocki commented Dec 4, 2020

@craicoverflow ACK. Generally good work that will allow us to stay in line with recent API so whatever it will arrive. As for non mocked usage for credentials we can put extra message for 404

Looks like you trying to use API that was not enabled yet in your current environment.

response, _, err := client.DefaultApi.CreateServiceAccount(context.Background())

t := time.Now()
serviceAcct := &managedservices.ServiceAccountRequest{Name: fmt.Sprintf("srvc-acct-%v", t.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.

Do we want the user to be able to name their service accounts too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes.

@craicoverflow craicoverflow force-pushed the credentials-api branch 2 times, most recently from bcd41b6 to e6a931d Compare December 7, 2020 12:26
@wtrocki
Copy link
Collaborator

wtrocki commented Dec 7, 2020

Rebase recomended

@craicoverflow craicoverflow changed the title [WIP] feat: update OPENAPI spec for Service Account feat: update OPENAPI spec for Service Account Dec 7, 2020
@wtrocki
Copy link
Collaborator

wtrocki commented Dec 7, 2020

WDYT about?
rhoas service-account vs rhoas serviceAccount vs rhoas serviceaccount

https://kubernetes.io/docs/reference/generated/kubectl/kubectl-commands#-em-serviceaccount-em-


// define api
const api = new OpenAPIBackend({ definition: path.join(__dirname, "../managed-services-api.yaml") });
const api = new OpenAPIBackend({ definition: path.join(__dirname, "../../openapi/managed-services-api.yaml") });
Copy link
Collaborator

Choose a reason for hiding this comment

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

The pattern was that we always have managed-services that reflect top level and mock could have our own additions. Cool with this approach as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can do that. But how did you generate the MAS client with two different API spec files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could overwrite it, but then the managed services client not being representative of the OpenAPI spec anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

All good here. I was just trying to tell how this worked. It is better this way and by the end of the week we should have API in master.

Copy link
Contributor Author

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

WDYT about?
rhoas service-account vs rhoas serviceAccount vs rhoas serviceaccount

kubernetes.io/docs/reference/generated/kubectl/kubectl-commands#-em-serviceaccount-em-

I'm not a fan of camelCase in a CLI, and it is not usual practice. Best to stick with kubectl/oc norms so I will use serviceaccount

@craicoverflow craicoverflow requested a review from wtrocki December 7, 2020 14:22
@wtrocki
Copy link
Collaborator

wtrocki commented Dec 7, 2020

I'm not a fan of camelCase in a CLI, and it is not usual practice. Best to stick with kubectl/oc norms so I will use serviceaccount

Yep. I have no strong opinion either. Just wanted for us to pick something. Best to not have - in name as those are reserved to arguments.

@craicoverflow
Copy link
Contributor Author

Best to not have - in name as those are reserved to arguments.

Not sure what you mean by that.

@wtrocki
Copy link
Collaborator

wtrocki commented Dec 7, 2020

command --argument-one

@craicoverflow
Copy link
Contributor Author

Yes but..hyphens are not purely reserved for flags/args. Only if the word begins with a hyphen then yes. Without hyphens longer commands would be hard to read. Anyway, we are not using it due to Kubectl style so this is a non-issue.

Copy link
Collaborator

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

Good to merge!

@craicoverflow craicoverflow merged commit c9805af into master Dec 7, 2020
@craicoverflow craicoverflow deleted the credentials-api branch December 7, 2020 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adapt to new credentials API

2 participants