[metricbeat] replace depricated io/ioutil #36963

florianl · 2023-10-25T16:51:51Z

Proposed commit message

As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred. Successor functionality in io or os is more performant than the deprecated functionality in io/ioutil.

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.next.asciidoc or CHANGELOG-developer.next.asciidoc.~~

mergify · 2023-10-25T16:52:49Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @florianl? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2023-10-25T18:26:55Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-11-01T06:56:39.800+0000
Duration: 80 min 20 sec

Test stats 🧪

Test	Results
Failed	0
Passed	5863
Skipped	1160
Total	7023

💚 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 and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

faec · 2023-10-25T20:16:29Z

metricbeat/module/openmetrics/collector/_meta/data.json

        },
        "metrics": {
-            "up": 1
-        }
+            "net_conntrack_listener_conn_closed_total": 0


Was this included by accident? Everything else in the PR looks like natural in-place substitutions, but this seems unrelated. (If the data.json changes weren't here I would approve it.)

sorry - I must have added them by accident and will remove them.

file got removed from the PR.

As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred. Signed-off-by: Florian Lehner <florian.lehner@elastic.co>

belimawr

Great job! I'm always happy to see this kin of improvements ❤️

The only blocking suggestion is the assert/require one.

belimawr · 2023-10-27T07:00:58Z

metricbeat/helper/server/http/http_test.go

+		t.Error(err)
+		t.FailNow()


That can be replaced by a single t.Fatal()

Suggested change

t.Error(err)

t.FailNow()

t.Fatal(err)

belimawr · 2023-10-27T07:01:23Z

metricbeat/mb/testing/testdata.go

+		t.Error(err)
+		t.FailNow()


Same here

Suggested change

t.Error(err)

t.FailNow()

t.Fatal(err)

Looking at existing code, t.Error(err) followed by t.FailNow() is the most common pattern. So far, no one cared that the error was ignored so far. And the change was added, just to make linter happy.

Looking at existing code, t.Error(err) followed by t.FailNow() is the most common pattern.

The bits of the codebase I mostly work with follow a different pattern 😅 , anyway this one is not a blocker.

belimawr · 2023-10-27T07:03:31Z

metricbeat/module/ceph/monitor_health/monitor_health_test.go

@@ -36,7 +36,8 @@ func TestFetchEventContents(t *testing.T) {
 	absPath, err := filepath.Abs("../_meta/testdata/")
 	assert.NoError(t, err)

-	response, err := ioutil.ReadFile(absPath + "/sample_response.json")
+	response, err := os.ReadFile(absPath + "/sample_response.json")
+	assert.NoError(t, err)


It's better to use require here as it will immediately fail the test. If we fail to read the file, there is no point continuing with the test.

Suggested change

assert.NoError(t, err)

require.NoError(t, err)

What is the difference to line 37 assert.NoError(t, err) ? Why should we use here require instead of assert?

require calls t.Fatal or t.FailNow which effectively interrupts the test right when the assertion fails. In that case if we continue the test after os.ReadFile fails, the test will only produce more errors that are not helpful, hence stopping the test when the file cannot be read is the best approach in my opinion.

That is the only comment/change I consider a blocker, and even that is not a hard blocker.

That is the only comment/change I consider a blocker, and even that is not a hard blocker.

I can't follow why this is considered a blocker. Before this change err was never checked before and the discussion assert vs require is a style discussion, that should not affect this PR.

metricbeat/module/nats/connection/connection_test.go

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>

florianl · 2023-12-14T07:41:58Z

satisfying linter is a major hurdle to get this merged. as I don't have capacity to do so, I'm closing this PR.

florianl added cleanup release-note:skip The PR should be ignored when processing the changelog labels Oct 25, 2023

florianl requested review from a team as code owners October 25, 2023 16:51

florianl requested review from belimawr and faec October 25, 2023 16:51

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 25, 2023

florianl requested review from gizas and MichaelKatsoulis October 25, 2023 16:51

florianl added the Team:Elastic-Agent Label for the Agent team label Oct 25, 2023

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 25, 2023

mergify bot assigned florianl Oct 25, 2023

florianl added the backport-skip Skip notification from the automated backport with mergify label Oct 25, 2023

faec reviewed Oct 25, 2023

View reviewed changes

[metricbeat] replace depricated io/ioutil

fedbe66

As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred. Signed-off-by: Florian Lehner <florian.lehner@elastic.co>

florianl force-pushed the flo-metricbeat-ioutil branch from 0032bda to fedbe66 Compare October 26, 2023 05:51

faec approved these changes Oct 26, 2023

View reviewed changes

belimawr requested changes Oct 27, 2023

View reviewed changes

florianl requested a review from belimawr October 27, 2023 08:33

belimawr approved these changes Oct 31, 2023

View reviewed changes

MichaelKatsoulis approved these changes Oct 31, 2023

View reviewed changes

klacabane approved these changes Oct 31, 2023

View reviewed changes

gizas reviewed Oct 31, 2023

View reviewed changes

metricbeat/module/nats/connection/connection_test.go Outdated Show resolved Hide resolved

gizas approved these changes Oct 31, 2023

View reviewed changes

remove missed io/ioutil from metricbeat

937a28b

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>

make linter happy

a135d00

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>

florianl closed this Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[metricbeat] replace depricated io/ioutil #36963

[metricbeat] replace depricated io/ioutil #36963

florianl commented Oct 25, 2023

mergify bot commented Oct 25, 2023

elasticmachine commented Oct 25, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

faec Oct 25, 2023

florianl Oct 26, 2023

florianl Oct 26, 2023

belimawr left a comment

belimawr Oct 27, 2023

belimawr Oct 27, 2023

florianl Oct 27, 2023

belimawr Oct 31, 2023

belimawr Oct 27, 2023

florianl Oct 27, 2023

belimawr Oct 31, 2023

florianl Oct 31, 2023

florianl commented Dec 14, 2023

[metricbeat] replace depricated io/ioutil #36963

[metricbeat] replace depricated io/ioutil #36963

Conversation

florianl commented Oct 25, 2023

Proposed commit message

Checklist

mergify bot commented Oct 25, 2023

elasticmachine commented Oct 25, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belimawr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florianl commented Dec 14, 2023

elasticmachine commented Oct 25, 2023 •

edited by jenkins-beats-ci bot

Loading