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

Fixes EIMP lint errors as per the latest OTel version #38

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

lahsivjar
Copy link
Contributor

@lahsivjar lahsivjar commented Jul 18, 2024

Fixes the lint failures as a result of update to v0.105.0. Most of the code is auto-generated other than the following changes:

  1. The actual EIMP codebase (not the generated ones) change for CreateSettings -> Settings.
  2. OCB version bump up for the testing distro.

I also noticed that lints are not run as part of the GH checks, this is what caused the lint error to go unnoticed. I will create a followup for this. This is not correct, not sure why the errors were not reported with the dependabot PR.

@lahsivjar lahsivjar requested a review from ishleenk17 July 18, 2024 10:18
@lahsivjar lahsivjar requested a review from a team as a code owner July 18, 2024 10:18
@lahsivjar lahsivjar requested a review from gregkalapos July 18, 2024 10:18
@ishleenk17
Copy link
Contributor

@lahsivjar : If this processor is used with components on version lesser than v0.105.0.
Would it cause any compatibility issue?

@lahsivjar
Copy link
Contributor Author

@ishleenk17 CreateSettings was deprecated in v0.103.0 and that's when the Settings was introduced. So, it would probably be incompatible with versions before v0.103.0.

I might be mistaken here but is this an issue for us? Don't we have full control over which version of the component we will package in our distro?

@lahsivjar
Copy link
Contributor Author

BTW, if the CreateSettings -> Settings is concerning for any reason then it should be okay to keep the CreateSettings. The main cause of test failures are UnmarshalConfig in the generated tests.

Copy link
Contributor

@rogercoll rogercoll left a comment

Choose a reason for hiding this comment

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

LGTM

@rogercoll
Copy link
Contributor

@ishleenk17

If this processor is used with components on version lesser than v0.105.0. Would it cause any compatibility issue?

I don't think so because the NewFactory method that is the one used in the distribution has no changes. We could also release patch version from v0.7.1.

I have just modified the sample manifest of this branch with versions lower than v0.105.0 for the other components and compilation works accordingly:

  otelcol_version: 0.105.0

extensions:

connectors:

converters:

receivers:
  - gomod: go.opentelemetry.io/collector/receiver/nopreceiver v0.101.0

processors:
  - gomod: github.com/elastic/opentelemetry-collector-components/processor/elasticinframetricsprocessor v0.0.0

exporters:
  - gomod: go.opentelemetry.io/collector/exporter/nopexporter v0.101.0

@lahsivjar
Copy link
Contributor Author

lahsivjar commented Jul 19, 2024

@rogercoll

otelcol_version: 0.105.0

If you keep the otelcol_version as > v0.103.0 then you won't have failures but if you use anything below that it should fail I think (since before that version the Settings struct does not exist).

I will wait for this discussion to conclude before merging. Alternatively, we can merge #40 which would fix the test but not the lint.

@lahsivjar
Copy link
Contributor Author

I might be mistaken here but is this an issue for us? Don't we have full control over which version of the component we will package in our distro?

Going back to this, why would we ever publish your distro with older version of the components? I thought we had full control over the versions we would ship with the distro - if this is true then I don't see any issues with maintaining backward compatibilities.

@ishleenk17 I saw that you approved the PR, does the above resolve the backward compatibility concern?

@ishleenk17
Copy link
Contributor

I might be mistaken here but is this an issue for us? Don't we have full control over which version of the component we will package in our distro?

Going back to this, why would we ever publish your distro with older version of the components? I thought we had full control over the versions we would ship with the distro - if this is true then I don't see any issues with maintaining backward compatibilities.

@ishleenk17 I saw that you approved the PR, does the above resolve the backward compatibility concern?

Yes, considering that we will always have the latest otelcol version for our Distro, I have approved this.

@lahsivjar lahsivjar merged commit 50388ac into elastic:main Jul 19, 2024
11 checks passed
@lahsivjar lahsivjar deleted the fix-eimp branch July 19, 2024 09:23
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