certificate manager #599
certificate manager #599
Conversation
Change provisioner.proto to match protobuf style guide.
The full contents of the git repository @432b2356ecb... was copied. Only go.mod was removed from it.
Apparently the existing library works out of the box, after all. We'll have to see how it works out continuing forward.
github-actions
bot
added
the
needs triage
| if p, err = h.auth.LoadProvisionerByID(id); err != nil { | ||
| api.WriteError(w, admin.WrapErrorISE(err, "error loading provisioner %s", id)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Same comment as before, do we need the id? should we ignore requests where id and name do not match?
There was a problem hiding this comment.
Same answer as before. It's not a name AND an ID. It's either one or the other. If a query parameter is submitted, the name in the URL is completely ignored.
There was a problem hiding this comment.
Is there any benefit in the API in allowing 2 unique identifiers (name and UUID)?
There was a problem hiding this comment.
Having to unique identifiers at this level is a little upsetting. At the db layer makes total sense.
| api.WriteError(w, admin.WrapErrorISE(err, "error retrieving paginated admins")) | ||
| return | ||
| } | ||
| api.JSON(w, &GetAdminsResponse{ |
There was a problem hiding this comment.
Protojson here? the problem with protojson, is that GetAdminsResponse is not a photo.Message, so I'm not sure if you can use it.
There was a problem hiding this comment.
Can't use it. Throws an error.
There was a problem hiding this comment.
Makes sense, a solution to this, to get a nicer JSON is to use a proto message like the AdminList. With a new cursor property.
| api.JSON(w, &GetProvisionersResponse{ | ||
| Provisioners: p, | ||
| NextCursor: next, | ||
| }) |
There was a problem hiding this comment.
Provisioners won't be nicely marshaled without using protojson.
There was a problem hiding this comment.
The provisioners in this list aren't protojson linkedca provisioners anyway. They're the certificates type of provisioner. This method isn't being used right now. We're still using the existing API with step ca provisioner list.
I can take this method out for now if you want.
There was a problem hiding this comment.
Ok for now, but to get a nicer JSON the same that we can use ProvisionerList with a cursor.
It's also possible to add custom MarshalJSON and UnmarshalJSON to GetProvisionersResponse that uses protojson to marshal the list of Provisioners/Admins.
- added a bit of validation to admin create and update - using protojson where possible in admin api - fixing a few instances of admin -> acme in errors
|
Update pushed. I addressed those comments which I could easily fix. Take a look when you have a chance and let me know. |
No description provided.