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.

Sign up for GitHub

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

Jump to bottom

Updating Elastic Manifests with NETINFO variable #3354

Merged
merged 8 commits into from
Sep 11, 2023
Merged

Updating Elastic Manifests with NETINFO variable #3354

merged 8 commits into from
Sep 11, 2023

Conversation

gizas
Copy link
Contributor

@gizas gizas commented Sep 5, 2023 •
edited
Loading

  • Enhancement

What does this PR do?

This PR add the the NETINFO variable in the kubernetes manifests

# The following NETINNFO:false variable will disable the netinfo.enabled option of add-host-metadata processor. This will remove fields host.ip and host.mac.  
            # For more info: https://www.elastic.co/guide/en/beats/metricbeat/current/add-host-metadata.html
            # - name: NETINFO
            #   value: "false"

Why is it important?

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

How to test this PR locally

cd elastic-agent
cd deploy/kubernetes
GENERATEKUSTOMIZE=true make ci-create-kustomize
make generate-k8s

Related issues

@gizas gizas requested review from a team as code owners September 5, 2023 13:38
@mergify mergify bot assigned gizas Sep 5, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 5, 2023

This pull request does not have a backport label. Could you fix it @gizas? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip label Sep 5, 2023
@elasticmachine
Copy link
Contributor

elasticmachine commented Sep 5, 2023 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-09-11T08:17:01.234+0000

  • Duration: 30 min 52 sec

Test stats 🧪

Test Results
Failed 0
Passed 6285
Skipped 59
Total 6344

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages.

  • run integration tests : Run the Elastic Agent Integration tests.

  • run end-to-end tests : Generate the packages and run the E2E Tests.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

This was referenced Sep 5, 2023
Closed
Open
@elasticmachine
Copy link
Contributor

elasticmachine commented Sep 5, 2023 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.78% (81/82) 👍
Files 66.212% (194/293) 👍
Classes 65.562% (356/543) 👍
Methods 52.627% (1122/2132) 👍
Lines 38.257% (12791/33434) 👍 0.018
Conditionals 100.0% (0/0) 💚

ChrsMark
ChrsMark reviewed Sep 7, 2023
View reviewed changes
...stic-agent-kustomize/default/elastic-agent-managed/base/elastic-agent-managed-daemonset.yaml Outdated
# The following ELASTIC_NETINFO:false variable will disable the netinfo.enabled option of add-host-metadata processor. This will remove fields host.ip and host.mac.
# For more info: https://www.elastic.co/guide/en/beats/metricbeat/current/add-host-metadata.html
# - name: ELASTIC_NETINFO
# value: "false"
Copy link
Member

@ChrsMark ChrsMark Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Shall we make it true false by default and uncommented based on the discussions at elastic/beats#36506 (comment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting default value ELASTIC_NETINFO: false as per discussion here elastic/beats#36506 (comment)

ChrsMark
ChrsMark approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Change LGTM. Let's wait for elastic/beats#36506 to be merged first, just in case :).

cmacknz
cmacknz reviewed Sep 7, 2023
View reviewed changes
changelog/fragments/1693920861-netinfo-manifests.yaml Outdated
kind: enhancement

# Change summary; a 80ish characters long description of the change.
summary: netinfo-manifests
Copy link
Member

Choose a reason for hiding this comment

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

You should explain what the actual change does that users can understand it, this is a change in the data that is collected so we shouldn't be brief.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Craig, see update debff38

@gizas
Copy link
Contributor Author

gizas commented Sep 11, 2023

/test

1 similar comment
@pierrehilbert
Copy link
Contributor

/test

@elastic-sonarqube
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@pierrehilbert
Copy link
Contributor

Unrelated failures, I'm merging.

@pierrehilbert pierrehilbert merged commit 2a7f5d0 into main Sep 11, 2023
24 of 28 checks passed
@pierrehilbert pierrehilbert deleted the NETINFOmanifests branch September 11, 2023 11:55
@gizas gizas mentioned this pull request Sep 11, 2023
Merged
gizas added a commit to elastic/kibana that referenced this pull request Sep 18, 2023
@gizas
Set env variable ELASTIC_NETINFO:false in Kibana (#166156)
dcd3a47 
## Summary

This PR add the environmental veriable ELASTIC_NETINFO in the managed
and standalone manifests of Elasitc agent.

The variable has been introduced here
elastic/elastic-agent#3354

The reason for the introduction of the new variable
ELASTIC_NETINFO:false by default in the manifests, is related with the
work done elastic/integrations#6674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmacknz cmacknz cmacknz approved these changes

@ChrsMark ChrsMark ChrsMark approved these changes

@tetianakravchenko tetianakravchenko Awaiting requested review from tetianakravchenko tetianakravchenko is a code owner automatically assigned from elastic/obs-cloudnative-monitoring

@ycombinator ycombinator Awaiting requested review from ycombinator ycombinator is a code owner automatically assigned from elastic/elastic-agent-control-plane

@faec faec Awaiting requested review from faec faec is a code owner automatically assigned from elastic/elastic-agent-control-plane

Assignees

@gizas gizas

Labels
backport-skip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gizas @elasticmachine @pierrehilbert @cmacknz @ChrsMark