Skip to content

Conversation

@efd6
Copy link
Contributor

@efd6 efd6 commented Sep 28, 2022

This renders the difference between the stored and rendered readme files. Like so:

[2022-09-27T07:35:28.126Z] README.md is outdated. Rebuild the package with 'elastic-package build'
[2022-09-27T07:35:28.126Z]   strings.Join({
[2022-09-27T07:35:28.126Z]   	... // 49896 identical bytes
[2022-09-27T07:35:28.126Z]   	"| host.os.name | Operating system name, without the version. | k",
[2022-09-27T07:35:28.126Z]   	"eyword |\n| host.os.name.text | Multi-field of `host.os.name`. | ",
[2022-09-27T07:35:28.126Z] - 	"match_only_",
[2022-09-27T07:35:28.126Z]   	"text |\n| host.os.platform | Operating system platform (such cent",
[2022-09-27T07:35:28.126Z]   	"os, ubuntu, windows). | keyword |\n| host.os.version | Operating ",
[2022-09-27T07:35:28.126Z]   	... // 10306 identical bytes
[2022-09-27T07:35:28.126Z]   }, "")
[2022-09-27T07:35:28.126Z] Error: checking package failed: checking readme files are up-to-date failed: files do not match

This is important due to differential behaviour between CI and local elastic-package build.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 28, 2022

💚 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: 2022-09-28T12:06:23.057+0000

  • Duration: 32 min 50 sec

Test stats 🧪

Test Results
Failed 0
Passed 831
Skipped 0
Total 831

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 28, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (34/34) 💚
Files 66.142% (84/127) 👍
Classes 61.111% (110/180) 👍
Methods 48.77% (357/732) 👍
Lines 32.097% (3218/10026) 👎 -0.038
Conditionals 100.0% (0/0) 💚

@efd6
Copy link
Contributor Author

efd6 commented Sep 28, 2022

/test

@jsoriano jsoriano requested a review from a team September 28, 2022 08:49
@jsoriano
Copy link
Member

This is important due to differential behaviour between CI and local elastic-package build.

To what different behavior do you refer?

When this error happens, locally you can run elastic-package build, and see with git diff what are the changes.

[2022-09-27T07:35:28.126Z] README.md is outdated. Rebuild the package with 'elastic-package build'
[2022-09-27T07:35:28.126Z] strings.Join({
[2022-09-27T07:35:28.126Z] ... // 49896 identical bytes
[2022-09-27T07:35:28.126Z] "| host.os.name | Operating system name, without the version. | k",
[2022-09-27T07:35:28.126Z] "eyword |\n| host.os.name.text | Multi-field of host.os.name. | ",
[2022-09-27T07:35:28.126Z] - "match_only_",

Is it expected for this diff to show the strings.Join(...... text? This looks like non evaluated Go code. Would it be possible to show a unified diff?

@efd6
Copy link
Contributor Author

efd6 commented Sep 28, 2022

At the moment the builds on CI are consistently rendering readme files differently to how they are rendered locally.

The diff is as it's intended to be, I would not like to see huge diffs here, just enough to be able to get an idea of what is being misrendered.

Docs for the format are here https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff.

@jsoriano
Copy link
Member

/test

@efd6
Copy link
Contributor Author

efd6 commented Sep 28, 2022

If you would prefer, we can do what we have in the pipeline result diffing

	var buf bytes.Buffer
	err = difflib.WriteUnifiedDiff(&buf, difflib.UnifiedDiff{
		A:        difflib.SplitLines(string(existing)),
		B:        difflib.SplitLines(string(rendered)),
		FromFile: "want",
		ToFile:   "got",
		Context:  3, // Maybe less context though.
	})
	return false, buf.String(), err

@jsoriano
Copy link
Member

Yes, I think I would prefer a more standard diff, could you give it a try and show an example?

@efd6
Copy link
Contributor Author

efd6 commented Sep 28, 2022

Sure. I'm thinking a context of 2 would be enough for this situation. WDYT?

@jsoriano
Copy link
Member

Agree with a small diff 👍

@efd6
Copy link
Contributor Author

efd6 commented Sep 28, 2022

I tried 2 and I think it's still too much. This is 1:

Format the package
Done
Lint the package
buildmanifest.Dependencies{ECS:buildmanifest.ECSDependency{Reference:"git@v8.4.0-rc1"}}
README.md is outdated. Rebuild the package with 'elastic-package build'
--- want
+++ got
@@ -308,3 +308,2 @@
 | rsa.internal.cid | This is the unique identifier used to identify a NetWitness Concentrator. This key should never be used to parse Meta data from a session (Logs/Packets) Directly, this is a Reserved key in NetWitness | keyword |
-| rsa.internal.data | Deprecated key defined only in table map. | keyword |
 | rsa.internal.dead | Deprecated key defined only in table map. | long |
@@ -342,4 +341,2 @@
 | rsa.internal.msg | This key is used to capture the raw message that comes into the Log Decoder | keyword |
-| rsa.internal.msg_id | This is the Message ID1 value that identifies the exact log parser definition which parses a particular log session. This key should never be used to parse Meta data from a session (Logs/Packets) Directly, this is a Reserved key in NetWitness | keyword |
-| rsa.internal.msg_vid | This is the Message ID2 value that identifies the exact log parser definition which parses a particular log session. This key should never be used to parse Meta data from a session (Logs/Packets) Directly, this is a Reserved key in NetWitness | keyword |
 | rsa.internal.node_name | Deprecated key defined only in table map. | keyword |
@@ -347,3 +344,2 @@
 | rsa.internal.obj_id | Deprecated key defined only in table map. | keyword |
-| rsa.internal.obj_server | Deprecated key defined only in table map. | keyword |
 | rsa.internal.obj_val | Deprecated key defined only in table map. | keyword |
Error: checking package failed: checking readme files are up-to-date failed: files do not match

@jsoriano jsoriano merged commit a2ea875 into main Sep 28, 2022
@jsoriano jsoriano deleted the diff-render branch September 28, 2022 14:08
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