Split `spec_service_binding.go` by app type: `Spring Boot app`, `other app` by rujche · Pull Request #105 · azure-javaee/azure-dev

rujche · 2025-01-14T13:41:49Z

Split spec_service_binding.go by app type: Spring Boot app, other app.

Copilot

Copilot reviewed 6 out of 14 changed files in this pull request and generated 1 comment.

Files not reviewed (8)

cli/azd/resources/scaffold/templates/resources.bicept: Language not supported
cli/azd/internal/scaffold/spec_service_binding_test.go: Evaluated as low risk
cli/azd/internal/repository/app_init.go: Evaluated as low risk
cli/azd/internal/repository/infra_confirm.go: Evaluated as low risk
cli/azd/internal/repository/infra_confirm_test.go: Evaluated as low risk
cli/azd/internal/scaffold/spec.go: Evaluated as low risk
cli/azd/internal/scaffold/bicep_env_test.go: Evaluated as low risk
cli/azd/internal/scaffold/bicep_env.go: Evaluated as low risk

Comments suppressed due to low confidence (4)

cli/azd/internal/binding/binding_test.go:151

The test case 'invalid input' in 'TestToTargetAndInfoType' should have an actual invalid input to test the function's behavior with invalid data.

input: "${binding:azure.db.mysql::username}"

cli/azd/internal/binding/binding_common.go:22

The error message should use 'common source' instead of 'spring boot app' to match the context.

return nil, fmt.Errorf("unsupported target type when binding for spring boot app, target.Type = %s", target.Type)

cli/azd/pkg/project/scaffold_gen.go:207

Ensure that the changes involving the merging of environment variables using MergeMapWithDuplicationCheck are covered by tests. This includes scenarios with and without duplication.

serviceSpec.Envs, err = binding.MergeMapWithDuplicationCheck(serviceSpec.Envs, projectConfig.Services[resource.Name].Env)

cli/azd/pkg/project/scaffold_gen.go:255

Ensure that the changes involving the handling of different source types (e.g., Spring Boot apps) are covered by tests. This includes verifying that the correct environment variables are returned for each source type.

sourceType := toSourceType(userService.Language)

…pec_service_binding

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Copilot reviewed 6 out of 14 changed files in this pull request and generated no comments.

Files not reviewed (8)

cli/azd/resources/scaffold/templates/resources.bicept: Language not supported
cli/azd/internal/scaffold/spec_service_binding_test.go: Evaluated as low risk
cli/azd/internal/repository/app_init.go: Evaluated as low risk
cli/azd/internal/repository/infra_confirm.go: Evaluated as low risk
cli/azd/internal/repository/infra_confirm_test.go: Evaluated as low risk
cli/azd/internal/scaffold/spec.go: Evaluated as low risk
cli/azd/internal/scaffold/bicep_env_test.go: Evaluated as low risk
cli/azd/internal/scaffold/bicep_env.go: Evaluated as low risk

Comments suppressed due to low confidence (2)

cli/azd/internal/binding/binding_spring_boot.go:1

The new functions for generating Spring Boot environment variables should be covered by tests to ensure they work as expected.

package binding

cli/azd/internal/binding/binding_common.go:22

The error message incorrectly mentions "spring boot app." It should be updated to "common source."

return nil, fmt.Errorf("unsupported target type when binding for spring boot app, target.Type = %s", target.Type)

rujche · 2025-01-14T14:06:43Z

+func GetBindingEnvs(source Source, target Target) (map[string]string,
+	error) {
+	switch source.Type {
+	case Java, SpringBoot: // todo: support other Java types


This todo will not be done in current PR.

Copilot

Copilot reviewed 6 out of 14 changed files in this pull request and generated 1 comment.

Files not reviewed (8)

cli/azd/resources/scaffold/templates/resources.bicept: Language not supported
cli/azd/internal/scaffold/spec_service_binding_test.go: Evaluated as low risk
cli/azd/internal/repository/app_init.go: Evaluated as low risk
cli/azd/internal/repository/infra_confirm.go: Evaluated as low risk
cli/azd/internal/repository/infra_confirm_test.go: Evaluated as low risk
cli/azd/internal/scaffold/spec.go: Evaluated as low risk
cli/azd/internal/scaffold/bicep_env_test.go: Evaluated as low risk
cli/azd/internal/scaffold/bicep_env.go: Evaluated as low risk

Comments suppressed due to low confidence (4)

cli/azd/internal/binding/binding_spring_boot.go:93

The InfoType used for 'spring.data.mongodb.uri' should be 'InfoTypeConnectionString' instead of 'InfoTypeJdbcUrl'.

"spring.data.mongodb.uri": ToBindingEnv(target, InfoTypeJdbcUrl),

cli/azd/internal/binding/binding_common.go:22

The error message should be 'unsupported target type when binding for common source, target.Type = %s'.

return nil, fmt.Errorf("unsupported target type when binding for spring boot app, target.Type = %s", target.Type)

cli/azd/pkg/project/scaffold_gen.go:339

[nitpick] The variable name 'sourceType' is not very descriptive. It should be renamed to 'bindingSourceType'.

sourceType := toSourceType(userService.Language)

cli/azd/pkg/project/scaffold_gen.go:349

The changes to the 'printEnvListAboutUses' function introduce new behavior that is not covered by tests. Add tests to ensure this function works correctly with the new 'binding' package.

func printEnvListAboutUses(infraSpec *scaffold.InfraSpec, projectConfig *ProjectConfig, console input.Console, ctx context.Context) error {

…Delete related todo.

rujche · 2025-01-14T15:51:42Z

-	ServiceTypeHostContainerApp: {
-		ServiceBindingInfoTypeHost: "https://{{BackendName}}.${containerAppsEnvironment.outputs.defaultDomain}",
+	binding.AzureContainerApp: {
+		binding.InfoTypeHost: "'https://{{BackendName}}.${containerAppsEnvironment.outputs.defaultDomain}'",


FYI: Added ' because it's a string value.

…tityClientId" to make it easier to understand.

rujche · 2025-01-15T02:26:46Z

        run: |
          cd ./cli/azd
-          go test $(go list ./... | grep -v github.com/azure/azure-dev/cli/azd/test/functional) -cover -v
+          go test $(go list ./... | grep -v github.com/azure/azure-dev/cli/azd/test/functional | grep -v github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning/terraform) -cover -v


Ignore the test failure in current PR because it's not caused by current PR. The failure already exists in previous PR: #104

Maybe this failure can be fixed by merge upstream main, but I'm not sure. Anyway, skip related test in current PR.

saragluna · 2025-01-15T06:08:12Z


+func toSourceType(language appdetect.Language) binding.SourceType {
+	switch language {
+	case appdetect.Java:


Is the type SpringBoot being used?

Currently, application will not be detected as SpringBoot, but when an application be detected as SpringBoot, it will take effect. Keep SpringBoot type is just for future usage.

Delete SpringBoot type will make it confusing: An application has type Java, but is called binding_spring_boot.go.

saragluna · 2025-01-15T06:12:53Z

+func BindToPostgres(sourceType binding.SourceType, serviceSpec *ServiceSpec, postgres *DatabasePostgres) error {
 	serviceSpec.DbPostgres = postgres
-	envs, err := GetServiceBindingEnvsForPostgres(*postgres)
+	envs, err := binding.GetBindingEnvs(binding.Source{Type: sourceType},


So the workflow of these envs is like:

App detection to define the app types, aka language

Then in scaffold, we get the env list, based on the app type + the resource type

In the bicep files, we inject the envs

So the only thing to pass down is the app type?

App detection to define the app types, aka language

Currently, app type is decided by language, in the future, it can be decided by other information like framework dependency.

Then in scaffold, we get the env list, based on the app type + the resource type

Not only app type, there is some other informations like jsJms isKafka, SpringBootVersion

In the bicep files, we inject the envs

Yes.

So the only thing to pass down is the app type?

Not only the app type, all these information are necessary:

Source

AppType

metadata like: isJms, isKafka, SpringBootVersion

Target

Target Type

AuthType

saragluna · 2025-01-15T06:13:39Z

 }

-func BindToCosmosDb(serviceSpec *ServiceSpec, cosmos *DatabaseCosmosAccount) error {
+func BindToCosmosDb(sourceType binding.SourceType, serviceSpec *ServiceSpec, cosmos *DatabaseCosmosAccount) error {


Seems like these methods are very alike, can we reduce them?

Good point. Updated.

saragluna · 2025-01-15T06:15:02Z

+			case ResourceTypeMessagingKafka:
+				variables, err = binding.GetBindingEnvs(binding.Source{
+					Type:              sourceType,
+					IsSpringBootKafka: true,


No need to refactor in this PR, but isSpringBootKafka is also some footprint we want to reduce.

All binding related logic has been moved into binding package, so there should be some code to represent the information about is spring boot kafka.

How about keeping current change in current PR, if we want to reduce it, we can do it in another PR.

Updated. Moved IsSpringBootKafka from Source to Source.Metadata.

haoozhang · 2025-01-15T09:00:58Z

+
+func GetBindingEnvsForSpringBootToPostgresql(authType internal.AuthType) (map[string]string, error) {
+	target := Target{Type: AzureDatabaseForPostgresql}
+	switch authType {


Here we get binding envs based on the app type first, then in each func, we return them based on the auth type.

I have a question that, the different dependencies used also have impact on the binding envs, should return different Env Vars. For example, for the app type: SpringBoot and the authType: ConnectionString of Kafka,

if the dependency is spring-cloud-starter-stream-kafka (spring-cloud stream style), it should configure the properties like spring.cloud.stream.kafka.binder.brokers.

if the dependency is spring-kafka (pure spring), it should configure the property like spring.kafka.bootstrap-servers.

So how to deal with this case?

Good point.

This is a feature which is not implemented before current PR.

After current PR merged, this can be implemented by adding metadata in binding.Source.Metadata:

Now we already have these metadata:

…er' , which is different from specified PrincipalType 'ServicePrincipal'.

rujche · 2025-01-15T09:21:48Z

      }
      {
        principalId: principalId
-        principalType: 'ServicePrincipal'


FYI: This is a bug before current PR.

Error message:

UnmatchedPrincipalType: The PrincipalId 'xxx' has type 'User' , which is different from specified PrincipalType 'ServicePrincipal'.

…nformation in binding.go.

Copilot

Copilot reviewed 6 out of 16 changed files in this pull request and generated no comments.

Files not reviewed (10)

cli/azd/resources/scaffold/templates/resources.bicept: Language not supported
cli/azd/internal/scaffold/spec_service_binding_test.go: Evaluated as low risk
cli/azd/internal/scaffold/bicep_env_test.go: Evaluated as low risk
cli/azd/internal/binding/binding_common.go: Evaluated as low risk
cli/azd/internal/repository/infra_confirm.go: Evaluated as low risk
cli/azd/internal/repository/infra_confirm_test.go: Evaluated as low risk
cli/azd/pkg/project/scaffold_gen.go: Evaluated as low risk
cli/azd/internal/scaffold/scaffold.go: Evaluated as low risk
cli/azd/internal/repository/app_init.go: Evaluated as low risk
cli/azd/internal/scaffold/spec.go: Evaluated as low risk

Comments suppressed due to low confidence (2)

cli/azd/internal/binding/binding_spring_boot.go:94

The method name should be GetBindingEnvsForSpringBootToMongoDB for consistency.

func GetBindingEnvsForSpringBootToMongoDb(authType internal.AuthType) (map[string]string, error) {

cli/azd/internal/scaffold/bicep_env.go:31

The ReplaceBindingEnv function should use the correct indices for the substring replacement. The current implementation may not handle cases where the prefix and suffix are not found correctly.

bicepEnvValue = binding.ReplaceBindingEnv(value, bicepEnvValue)

rujche · 2025-01-20T09:29:15Z

Hi, @saragluna , @haoozhang .
Please help to review this PR again.

Split spec_service_binding.go by app type: Spring Boot app, other app.

7466e88

rujche requested review from Copilot and saragluna January 14, 2025 13:41

Copilot AI reviewed Jan 14, 2025

View reviewed changes

Comment thread cli/azd/internal/binding/binding_spring_boot.go Outdated

rujche and others added 2 commits January 14, 2025 21:44

Merge remote-tracking branch 'azure-javaee/feature/sjad' into split-s…

b979bc4

…pec_service_binding

Update cli/azd/internal/binding/binding_spring_boot.go

ce86fed

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

rujche requested review from Copilot and haoozhang January 14, 2025 13:45

Copilot AI reviewed Jan 14, 2025

View reviewed changes

Fix test failure in infra_confirm_test.go.

6a95216

rujche commented Jan 14, 2025

View reviewed changes

rujche added 4 commits January 14, 2025 22:10

Update the text of test.

0ea4b48

Fix error in bicep_env.go

ad6cd7a

Fix error in bicep_env.go: "$service$name" -> "$service $name"

6e263c0

Delete "type Env" because it's not used anymore.

636d1fa

rujche requested a review from Copilot January 14, 2025 15:12

Copilot AI reviewed Jan 14, 2025

View reviewed changes

Comment thread cli/azd/internal/binding/binding.go Outdated

In bicep_env_test.go, add test: "Eureka server" and "Config server". …

0376e59

…Delete related todo.

rujche commented Jan 14, 2025

View reviewed changes

rujche added 8 commits January 14, 2025 23:58

Enhance test by renaming "configServerName" to "config-server-name"

bb5682d

Fix GitHub action failure reported by lint.

6f6e5d2

Skip test whose failure is not cause by current PR.

203cd4c

Fix test failure: Test_ImportManager_ProjectInfrastructure_FromResources

8d1453a

Delete "bindingEnvValuePrefix" because it's not used.

abe4b42

Rename "EnvManagedIdentityClientId" to "SourceUserAssignedManagedIden…

c487900

…tityClientId" to make it easier to understand.

Add unit test in TestIsBindingEnvValue, And fix test failure.

8905e23

Improve ToBicepEnv, and add unit test.

8d880d4

rujche commented Jan 15, 2025

View reviewed changes

rujche added 2 commits January 15, 2025 10:57

Change placeholder value to make it easier to understand.

79d5a0f

Create func: isPlaceholderOfSourceClientId

2a19da5

saragluna reviewed Jan 15, 2025

View reviewed changes

haoozhang reviewed Jan 15, 2025

View reviewed changes

Fix error: UnmatchedPrincipalType: The PrincipalId 'xxx' has type 'Us…

19a6473

…er' , which is different from specified PrincipalType 'ServicePrincipal'.

rujche commented Jan 15, 2025

View reviewed changes

rujche added 3 commits January 15, 2025 23:03

Merge branch 'azure-javaee/feature/sjad' into split-spec_service_binding

a601e70

Remove duplicated codes in spec_service_binding.go by adding a new func.

46e63cd

Use "map[MetadataType]string" to avoid contains spring boot related i…

45a49a7

…nformation in binding.go.

rujche marked this pull request as draft January 16, 2025 02:29

rujche marked this pull request as ready for review January 17, 2025 02:04

Merge branch 'azure-javaee/feature/sjad' into split-spec_service_binding

6fe9c3f

rujche requested review from Copilot, haoozhang and saragluna January 20, 2025 09:23

Copilot AI reviewed Jan 20, 2025

View reviewed changes

saragluna approved these changes Jan 22, 2025

View reviewed changes

rujche merged commit 1a5ffb5 into azure-javaee:feature/sjad Jan 23, 2025

rujche deleted the split-spec_service_binding branch January 23, 2025 08:08

Uh oh!

Conversation

rujche commented Jan 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

rujche commented Jan 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants