add asogroups #3574
add asogroups #3574
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
size/XL
| limitations under the License. | ||
| */ | ||
|
|
||
| package asogroups |
There was a problem hiding this comment.
suggestion for code organization: instead of prefixing every folder name with aso it might make sense to reorganize the services folder with two sub-folders: sdk and aso (we can even add sdkv2 if needed).
There was a problem hiding this comment.
I think this will be the only service to have both ASO and SDK versions in the repo at the same time. Once we're ready for more services, I think the way the proposal describes of transitioning each service in-place will be easiest. Do you think it's still important to distinguish that in the folder structure?
There was a problem hiding this comment.
I think so long as we're able to do a targeted, quick revert (ideally just click the github revert PR button) per service it shouldn't matter. If one or the other makes that easier then I'd vote for that.
Sorry this didn't get hashed out in the proposal PR, probably this is too low level for being declared in the proposal, I should have caught that, if so.
There was a problem hiding this comment.
@nojnhuh if it's a one-off and not long-lived then probably fine as-is
There was a problem hiding this comment.
@CecileRobertMichon Sounds good. We can always adjust later too if needed.
@jackfrancis I think reverting in this case would be as simple as reverting the relevant commits, but we'll verify that as part of #3546.
nojnhuh
force-pushed
the
aso-groups
branch
4 times, most recently
from
c3309aa to
9c07dc3
Compare
|
Sorry for the spam, I'm finally done fiddling with this. |
|
ok last one I promise 🤡 |
|
/lgtm Great work @nojnhuh !! 🚀 |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: a13754791644ff977fb7d48f7013622dde012aea
|
Right, this isn't hook up to anything yet so the existing groups service will be used. This branch on my fork contains a running list of everything needed to wire up all the ASO changes so far, but there's no simple toggle for that in these changes: main...nojnhuh:cluster-api-provider-azure:aso-wired. Maybe I could open a WIP PR for that branch to get test signal? |
Actually I'm pretty confident tests will fail with the current state of things, especially tests doing any kind of BYO resource and upgrade tests, so maybe it's best to defer running tests in CI until the ASO stuff is a bit more fleshed out (at least through #3521)? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds an ASO-backed implementation for the resource groups service.
The proposal in #3113 says that each service will be transitioned to ASO in one change where both an SDK- and ASO-backed implementation will not coexist at the same time. For only this first service, I'm opting to break that rule to better enable making further iterative progress on the common framework. This service is expected to evolve alongside those future changes until #3527 is complete.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #3524
Special notes for your reviewer:
This PR includes no functional changes. If you'd like to try these changes out though, here's a branch that's ready to use that makes these changes operative: main...nojnhuh:cluster-api-provider-azure:aso-wired
TODOs:
Release note: