-
Notifications
You must be signed in to change notification settings - Fork 12
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
TRACING-4824: add a way to detect components support level in upstream #104
Conversation
67e4d17
to
024dcd5
Compare
Signed-off-by: Israel Blancas <[email protected]>
024dcd5
to
181cf5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go.mod is missing
|
||
if _, err := os.Stat(filepath.Join(pwd, repo)); os.IsNotExist(err) { | ||
log.Printf("Repository %s does not exist, cloning...\n", repo) | ||
err := exec.Command("git", "clone", fmt.Sprintf("https://github.com/open-telemetry/%s.git", repo)).Run() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need to clone those repos? Could we not use the github api instead?
Pointing to some go.mod
file should be should be sufficient to extract the information from the metadata.yaml
files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, this method is simpler and easier to follow. I don't think we need that kind of optimization for this use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thing I see here is that the current approach is more error prune and someone needs to maintain it.
Also using exec instead of simply querying some API endpoints feels to me like bad practice.
Additonally we dont run into any issues, when the exec runs on mac or linux machines with different git versions or limitations on the executed processes.
As a starting point, all details are available in this manifest file:
https://raw.githubusercontent.com/open-telemetry/opentelemetry-collector-releases/refs/heads/main/distributions/otelcol-contrib/manifest.yaml
When iterating on the manifest details, we manly have to construct two types of domains...
Example:
- https://raw.githubusercontent.com/open-telemetry/opentelemetry-collector/refs/tags/exporter/debugexporter/v0.112.0/exporter/debugexporter/metadata.yaml
or - https://raw.githubusercontent.com/open-telemetry/opentelemetry-collector-contrib/refs/tags/v0.112.0/exporter/alertmanagerexporter/metadata.yaml
Finally merge all the metadata and render it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thing I see here is that the current approach is more error prune and someone needs to maintain it.
I don't still see why this is "more error-prone". A call to the GitHub API will need to be maintained too.
Also using exec instead of simply querying some API endpoints feels to me like bad practice.
I would agree if this would be an application run in production systems where we need high availability or something like that and not just an script to get what components are supported or not as part of our release process.
Additonally we dont run into any issues, when the exec runs on mac or linux machines with different git versions or limitations on the executed processes.
It's just "git clone". You should have it configured on your machine to be able to work.
As a starting point, all details are available in this manifest file:
https://raw.githubusercontent.com/open-telemetry/opentelemetry-collector-releases/refs/heads/main/distributions/otelcol-contrib/manifest.yaml
When iterating on the manifest details, we manly have to construct two types of domains...
Example:
https://raw.githubusercontent.com/open-telemetry/opentelemetry-collector/refs/tags/exporter/debugexporter/v0.112.0/exporter/debugexporter/metadata.yaml
or
https://raw.githubusercontent.com/open-telemetry/opentelemetry-collector-contrib/refs/tags/v0.112.0/exporter/alertmanagerexporter/metadata.yaml
Finally merge all the metadata and render it.
I don't get what you want to explain with this. Please, you could you detail it more?
Signed-off-by: Israel Blancas <[email protected]>
Output example: