-
Notifications
You must be signed in to change notification settings - Fork 28
Provide option to not regenerate gRPC auth token on monitor registration #370
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with confirmation about the server version gating
InstallationID *uuid.UUID `json:"InstallationId" validate:"required"` | ||
MachineID string `json:"MachineId" validate:"required"` | ||
SpaceID string `json:"SpaceId,omitempty"` | ||
PreserveAuthenticationToken *bool `json:"PreserveAuthenticationToken,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this is set to true
on a server that doesn't have it? Is there any way to gate this behind a server version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm we can grab this semVar from the root resource to compare I think?
type RootResource struct {
Application string `json:"Application" validate:"required"`
Version string `json:"Version" validate:"required"`
APIVersion string `json:"ApiVersion" validate:"required"`
InstallationID *uuid.UUID `json:"InstallationId" validate:"required"`
IsEarlyAccessProgram bool `json:"IsEarlyAccessProgram"`
HasLongTermSupport bool `json:"HasLongTermSupport"`
resources.Resource
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have a flag elsewhere... let me see if we can use it for parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure here - @zentron do you have any thoughts on the API compatibility here? Should we make a new request resource that's like a v2 request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen the latest TF code since the framework upgrade, but can we not just fail in the TF the version is lower than we expect and its set to a value.
Happy to let @mjhilton take the lead on preferred way to deal with schema changes in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the TF, yeah - we have options, but in the go-client itself, there's a few more intricacies. I'm gonna see if there's any docs on this anywhere...
https://github.com/OctopusDeploy/OctopusDeploy/pull/37244 implements PreserveAuthenticationToken, which provides the toggle for whether or not we regenerate gRPC auth token on monitor registration. This PR adds the go SDK support for it.