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

Fix reading dtconfig.json #37

Closed
wants to merge 1 commit into from
Closed

Conversation

Bataran
Copy link
Contributor

@Bataran Bataran commented Dec 18, 2023

In GCF (1st and 2nd gen functions) the source code of a function is placed at /workspace/serverless_function_source_code folder, but since the function is started from the parent /workspace folder reading the dtconfig.json at "./dtconfig.json" path is not working.
The PR fixes it by reading the file from ./serverless_function_source_code/dtconfig.json

@dynatrace-cla-bot
Copy link
Member

dynatrace-cla-bot commented Dec 18, 2023

CLA assistant check
All committers have signed the CLA.

@@ -58,7 +58,7 @@ type jsonConfigFileReader struct {
// ReadConfigFromFile looks for a config file "dtconfig.json" in the current directory and attempts to parse it.
// Returns an error if the file can't be read or the parsing fails.
func (j *jsonConfigFileReader) readConfigFromFile() (fileConfig, error) {
return j.readConfigFromFileByPath("./dtconfig.json")
return j.readConfigFromFileByPath("./serverless_function_source_code/dtconfig.json")
Copy link
Contributor

@Oberon00 Oberon00 Dec 18, 2023

Choose a reason for hiding this comment

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

It is unexpected that not obviously cloud-specific core functions use a hard-coded cloud-specific path. Can this maybe be used as a fallback? Or maybe we instead need instructions how to configure package.json to copy the dtconfig.json from source to build output (similar to .NET I believe)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the go exporter is only meant to be used in GCF, this is a quick fix to make it work in GCF only (at the moment it's only documented for GCF, but I believe in future this will be used in other clouds as well). Note that it will be improved in the next iteration so it handle the dtconfig.json more gracefully. I think there are few options here: 1. documenting how to set the path to the file 2. automatically set the path based on the cloud 3. something else

Copy link

Choose a reason for hiding this comment

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

IMHO both are valid points. In this PR, can it be changed to act like a fallback while keeping the current behaviour intact? I.e. look for "./dtconfig.json" first, if not found look for "./serverless_function_source_code/dtconfig.json"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in #38 , this PR is redundant and should not be merged. I'll decline it.

@Bataran Bataran closed this Dec 20, 2023
@Bataran Bataran deleted the mm/fix-dtconfig-file-location branch December 20, 2023 10:28
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.

4 participants