Skip to content
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

GODRIVER-2906 Add container Env to Handshake. #1382

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

qingyang-hu
Copy link
Collaborator

GODRIVER-2906

Summary

  • Update the metadata handshake client.env.name field to optional.
  • Add client.env.container.

The values for client.env.container are sourced from the environment, see here.

Background & Motivation

client.env.container.runtime is untested since it requires "/.dockerenv", which exists in the root directory.

@qingyang-hu qingyang-hu marked this pull request as ready for review September 14, 2023 12:43
@qingyang-hu qingyang-hu requested a review from a team as a code owner September 14, 2023 12:43
@qingyang-hu qingyang-hu requested review from blink1073 and prestonvasquez and removed request for a team September 14, 2023 12:43
@qingyang-hu qingyang-hu marked this pull request as draft September 14, 2023 16:22
@github-actions
Copy link

API Change Report

No changes found!

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -256,15 +292,11 @@ func appendClientEnv(dst []byte, omitNonName, omitDoc bool) ([]byte, error) {
}

name := getFaasEnvName()
if name == "" {
container := getContainerEnvInfo()
if name == "" && container == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line implies that we wouldn't append the client environment if the name and the container are unavailable. However, both name and container were made optional as part of DRIVERS-2570: https://github.com/mongodb/specifications/pull/1454/files#diff-0ecfac0976ff0bfe5b91b5bd52bc5a79dd1125434505ff22331c7cadf1b2f25eR170

I think we should completely remove this block.

If tests are failing, such as the handshake integration tests, it's because there is an expectation that they should contain a name which is now incorrect.

Copy link
Collaborator Author

@qingyang-hu qingyang-hu Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid it's ambiguous. My understanding is that the entire client.env can be omitted because:

  1. The spec states the env is optional.
  2. Logically, we don't have to keep the env if it has nothing.

However, I will ask for clarification because we don't have a lot of reference implementations now.

Spec says:

If no fields of client.env would be populated, client.env MUST be entirely omitted. ref

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, so because the other fields in env depend on either name or container, if both are empty then implicitly the other fields will be empty. Could we add a comment to that effect?

var idx int32
idx, dst = bsoncore.AppendDocumentElementStart(dst, "env")

if name != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of nesting these conditions, suggest making them separate blocks for readability:

	var idx int32
	idx, dst = bsoncore.AppendDocumentElementStart(dst, "env")

	if name != "" {
		dst = bsoncore.AppendStringElement(dst, "name", name)
	}

	if !omitNonName {
		switch name {
		case envNameAWSLambda:
			dst = addMem(envVarAWSLambdaFunctionMemorySize)
			dst = addRegion(envVarAWSRegion)
		case envNameGCPFunc:
			dst = addMem(envVarFunctionMemoryMB)
			dst = addRegion(envVarFunctionRegion)
			dst = addTimeout(envVarFunctionTimeoutSec)
		case envNameVercel:
			dst = addRegion(envVarVercelRegion)
		}
	}

Copy link
Collaborator

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@qingyang-hu qingyang-hu merged commit c3d390f into mongodb:v1 Sep 19, 2023
1 check failed
blink1073 pushed a commit to blink1073/mongo-go-driver that referenced this pull request Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants