-
Notifications
You must be signed in to change notification settings - Fork 171
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
Hive SyncSet GenevaActions #3729
base: master
Are you sure you want to change the base?
Conversation
328aa29
to
32565f2
Compare
@@ -124,6 +125,7 @@ func NewFrontend(ctx context.Context, | |||
clusterm metrics.Emitter, | |||
aead encryption.AEAD, | |||
hiveClusterManager hive.ClusterManager, | |||
hiveSyncSetManager hive.SyncSetManager, |
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.
Would it make more sense to add this using the builder pattern or something similar:https://medium.com/@reetas/clean-ways-of-adding-new-optional-fields-to-a-golang-struct-99ae2fe9719d?
What do you think?
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.
the rational for creating a new interface is bcoz the syncsets are not cluster scoped and hive instance scoped, so its cleaner to have a new interface which implements syncset functions.
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 have added my comments mostly for enhancement. However I'd like to request changes for the usage of HTTP 403 Forbidden status as I've specified. HTTP 400 Bad Request should be the right response for client requests that lack parameters or wrong parameters.
It would be better to refactor like https://github.com/Azure/ARO-RP/pull/3665/files first to focus on the actual change. |
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.
We want e2e tests for this feature.
Resolved, made the changes. |
Please rebase pull request. |
pkg/hive/generate.go
Outdated
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.
flagging this as needing a rebase on master to pick up the .bingo changes.
6bf8d67
to
fa9efc1
Compare
Please rebase pull request. |
Implements: 1. Geneva Action to list selectorsyncsets for a given hive instance 2. Geneva Action to retrieve an individual selectorsyncset on a given hive instance 3. Geneva Action to list syncsets for a cluster defined by resource id in a hive instance 4. Geneva Action to retrieve an individual syncsets on a given hive instance How to call: List selector/syncsets - /admin/hivesyncset?namespace={namespace}?isSyncset Get a selector/syncset - /admin/hivesyncset/syncsetname/{syncsetname}?namespace={namespace}?isSyncSet Notes: 1. namespace should be "" for selectorsyncsets 2. namespace cannot be "" for syncsets 3. isSyncSet can be true/false, setting it changes from using default selectorsyncset to syncset
fa9efc1
to
0ff61e8
Compare
/azp run ci |
Azure Pipelines successfully started running 1 pipeline(s). |
Implements:
How to call:
List selector/syncsets - /admin/hivesyncset?namespace={namespace}?isSyncset
Get a selector/syncset - /admin/hivesyncset/syncsetname/{syncsetname}?namespace={namespace}?isSyncSet
Notes:
Which issue this PR addresses:
Fixes
What this PR does / why we need it:
Test plan for issue:
Testing is underway., below is the plan.,
Is there any documentation that needs to be updated for this PR?
How do you know this will function as expected in production?