init: azure rbac aks service discovery #1311

girdharshubham · 2024-08-12T21:01:03Z

Checklist

Documentation
Tests
Core

...iscovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAKSServiceDiscovery.scala

johanandren

Looking good, added some feedback

akka-discovery-azure-api/src/main/resources/reference.conf

akka-discovery-azure-api/src/main/scala/akka/discovery/azureapi/Settings.scala

johanandren · 2024-08-13T12:24:45Z

...iscovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAKSServiceDiscovery.scala

+        port = maybePort,
+        address = Some(InetAddress.getByName(ip))
+      )
+    }


Would be nice if you could split this huge for comprehension into somewhat more digestible pieces, not exactly sure how, maybe some helper methods like runningPods(...), portFor(...) to get a bit higher level operations you can then use in a for comprehension?

...iscovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAKSServiceDiscovery.scala

johanandren · 2024-08-13T12:39:29Z

akka-discovery-azure-api/src/main/scala/akka/discovery/azureapi/JsonFormat.scala

+ * INTERNAL API
+ */
+@InternalApi private[azureapi] object JsonFormat extends SprayJsonSupport with DefaultJsonProtocol {
+  // If adding more formats here, remember to also add in META-INF/native-image reflect config


Not entirely sure we need to support native image with this module, but if we should, there should be a META-INF/native-image/com.lightbend.akka.management.akka-discovery-azure-api/reflect-config.json listing the model classes. You can look at the corresponding reflect-config.json in lease-kubernetes for an example.

jsonFormatN inspects them using reflection so must explicitly be made available for native image/graal through such config.

I plan on doing native image later in a different PR.

akka-discovery-azure-api/src/main/resources/reference.conf

patriknw

Looking good

build.sbt

...iscovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAKSServiceDiscovery.scala

discovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAksServiceDiscovery.scala

mblund

I read through the PR changes and gave some comments on the code.
Would probably have some way of testing this as well.

discovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAksServiceDiscovery.scala

mblund · 2024-08-13T17:50:47Z

discovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAksServiceDiscovery.scala

+    }
+  }
+
+  import system.dispatcher


Should probably be another dispatcher

I'm not sure about it 🤔

discovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAksServiceDiscovery.scala

discovery-azure-api/src/main/scala/akka/discovery/azureapi/JsonFormat.scala

add: License file refactor: general clean up

project/project-info.conf

patriknw

looking good, just something minor...

...azure-api/src/main/scala/akka/discovery/azureapi/rbac/aks/AzureRbacAksServiceDiscovery.scala

discovery-azure-api/src/test/scala/akka/discovery/azureapi/AzureApiSpec.scala

discovery-azure-api/src/test/scala/akka/discovery/azureapi/rbac/aks/JsonFormatSpec.scala

project/project-info.conf

build.sbt

Co-authored-by: Johan Andrén <[email protected]>

patriknw

LGTM, great work!

johanandren

LGTM, great work @girdharshubham

girdharshubham force-pushed the azure-rbac-aks-discovery branch 2 times, most recently from 56f0943 to 29cc384 Compare August 13, 2024 10:20

olofwalker reviewed Aug 13, 2024

View reviewed changes

...iscovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAKSServiceDiscovery.scala Outdated Show resolved Hide resolved

olofwalker reviewed Aug 13, 2024

View reviewed changes

...iscovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAKSServiceDiscovery.scala Outdated Show resolved Hide resolved

olofwalker reviewed Aug 13, 2024

View reviewed changes

...iscovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAKSServiceDiscovery.scala Outdated Show resolved Hide resolved

johanandren reviewed Aug 13, 2024

View reviewed changes

patriknw reviewed Aug 13, 2024

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

...iscovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAKSServiceDiscovery.scala Outdated Show resolved Hide resolved

girdharshubham force-pushed the azure-rbac-aks-discovery branch 2 times, most recently from b29c597 to cd52a08 Compare August 13, 2024 15:44

girdharshubham marked this pull request as ready for review August 13, 2024 15:44

girdharshubham force-pushed the azure-rbac-aks-discovery branch from cd52a08 to 2d02c43 Compare August 13, 2024 15:49

init: azure rbac aks service discovery

d304117

girdharshubham force-pushed the azure-rbac-aks-discovery branch from 2d02c43 to d304117 Compare August 13, 2024 15:54

resolve: pr comments for module name

00c4825

girdharshubham changed the title ~~--wip-- init: azure rbac aks service discovery~~ init: azure rbac aks service discovery Aug 13, 2024

girdharshubham commented Aug 13, 2024

View reviewed changes

discovery-azure-api/src/main/scala/akka/discovery/azureapi/AzureRbacAksServiceDiscovery.scala Outdated Show resolved Hide resolved

mblund reviewed Aug 13, 2024

View reviewed changes

girdharshubham force-pushed the azure-rbac-aks-discovery branch from 15839b5 to 76743d7 Compare August 14, 2024 12:41

docs: add docs snip for azure rbac aks api discovery

148202e

add: License file refactor: general clean up

girdharshubham force-pushed the azure-rbac-aks-discovery branch from 76743d7 to 148202e Compare August 15, 2024 12:52

girdharshubham added 4 commits August 15, 2024 23:06

docs: for azure rbac aks api

19ee526

test: for target list

10300c4

test: for azure rbac aks api

79606a7

resolve: pr comments and general clean up

dfb16ed

girdharshubham requested review from patriknw, mblund, johanandren and olofwalker August 16, 2024 09:59

girdharshubham commented Aug 16, 2024

View reviewed changes

project/project-info.conf Outdated Show resolved Hide resolved

clean up and scalafmt

c737e3c

patriknw reviewed Aug 16, 2024

View reviewed changes

girdharshubham added 2 commits August 16, 2024 21:57

resolve: pr comments from Patrik

a2f8a41

remove: unnecessary application.conf file

2137222

girdharshubham requested a review from patriknw August 16, 2024 16:36

set mimaPreviousArtifacts to empty

34ab91c

johanandren reviewed Aug 19, 2024

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

Update build.sbt

767ef6a

Co-authored-by: Johan Andrén <[email protected]>

girdharshubham requested a review from johanandren August 19, 2024 08:29

patriknw approved these changes Aug 19, 2024

View reviewed changes

johanandren approved these changes Aug 19, 2024

View reviewed changes

johanandren merged commit 6d28eae into akka:main Aug 19, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init: azure rbac aks service discovery #1311

init: azure rbac aks service discovery #1311

girdharshubham commented Aug 12, 2024 •

edited

Loading

johanandren left a comment

johanandren Aug 13, 2024

johanandren Aug 13, 2024

girdharshubham Aug 16, 2024

patriknw left a comment

mblund left a comment

mblund Aug 13, 2024 •

edited

Loading

girdharshubham Aug 16, 2024

patriknw left a comment

patriknw left a comment

johanandren left a comment

init: azure rbac aks service discovery #1311

init: azure rbac aks service discovery #1311

Conversation

girdharshubham commented Aug 12, 2024 • edited Loading

Checklist

johanandren left a comment

Choose a reason for hiding this comment

johanandren Aug 13, 2024

Choose a reason for hiding this comment

johanandren Aug 13, 2024

Choose a reason for hiding this comment

girdharshubham Aug 16, 2024

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

mblund left a comment

Choose a reason for hiding this comment

mblund Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

girdharshubham Aug 16, 2024

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

johanandren left a comment

Choose a reason for hiding this comment

girdharshubham commented Aug 12, 2024 •

edited

Loading

mblund Aug 13, 2024 •

edited

Loading