internal/{docs,testrunner/runners/system,internal/testrunner/runners/pipeline}: make json files end with newlines by efd6 · Pull Request #2087 · elastic/elastic-package

efd6 · 2024-09-06T09:24:12Z

Currently test expectations and sample events are rendered to files without a
trailing space. This causes unnecessary diff noise as various people add or
remove the trailing character. In the case of sample events, it also results
in the addition of blank lines in the sample events rendered in to the
readme.

This change ensures that all test expectations and sample events are rendered
with a final new line and that sample events are white-space trimmed before
insertion into readme files.

elasticmachine · 2024-09-06T09:30:18Z

💔 Build Failed

Buildkite Build
Commit: 04157c7

Failed CI Steps

History

cc @efd6

…pipeline}: make json files end with newlines

chrisberkhout

Maybe

elastic-package/internal/formatter/json.go

Lines 22 to 78 in ff78679

    
           func JSONFormatterBuilder(specVersion semver.Version) JSONFormatter { 
        
           	if specVersion.LessThan(semver.MustParse("2.12.0")) { 
        
           		return &jsonFormatterWithHTMLEncoding{} 
        
           	} 
        
           	return &jsonFormatter{} 
        
           } 
        
           // jsonFormatterWithHTMLEncoding function is responsible for formatting the given JSON input. 
        
           // It encodes special HTML characters. 
        
           type jsonFormatterWithHTMLEncoding struct{} 
        
           func (jsonFormatterWithHTMLEncoding) Format(content []byte) ([]byte, bool, error) { 
        
           	var rawMessage json.RawMessage 
        
           	err := json.Unmarshal(content, &rawMessage) 
        
           	if err != nil { 
        
           		return nil, false, fmt.Errorf("unmarshalling JSON file failed: %w", err) 
        
           	} 
        
           	formatted, err := json.MarshalIndent(&rawMessage, "", "    ") 
        
           	if err != nil { 
        
           		return nil, false, fmt.Errorf("marshalling JSON raw message failed: %w", err) 
        
           	} 
        
           	return formatted, string(content) == string(formatted), nil 
        
           } 
        
           func (jsonFormatterWithHTMLEncoding) Encode(doc any) ([]byte, error) { 
        
           	return json.MarshalIndent(doc, "", "    ") 
        
           } 
        
           // jsonFormatter function is responsible for formatting the given JSON input. 
        
           type jsonFormatter struct{} 
        
           func (jsonFormatter) Format(content []byte) ([]byte, bool, error) { 
        
           	var formatted bytes.Buffer 
        
           	err := json.Indent(&formatted, content, "", "    ") 
        
           	if err != nil { 
        
           		return nil, false, fmt.Errorf("formatting JSON document failed: %w", err) 
        
           	} 
        
           	return formatted.Bytes(), bytes.Equal(content, formatted.Bytes()), nil 
        
           } 
        
           func (jsonFormatter) Encode(doc any) ([]byte, error) { 
        
           	var formatted bytes.Buffer 
        
           	enc := json.NewEncoder(&formatted) 
        
           	enc.SetEscapeHTML(false) 
        
           	enc.SetIndent("", "    ") 
        
           	err := enc.Encode(doc) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	// Trimming to be consistent with MarshalIndent, that seems to trim the result. 
        
           	return bytes.TrimSpace(formatted.Bytes()), nil 
        
           }

is the better place?
If that's going impact too many things here seem okay.

efd6 · 2024-09-09T21:39:37Z

I looked at doing it there, but the convolution of behaviours made it harder to reason about, so I put it where it was easier to understand.

jsoriano · 2024-09-10T08:07:46Z

Hey @efd6, could you add a description for this change? Is it avoiding the need to reformat the files after generating them? 🤔

jsoriano · 2024-09-10T08:20:55Z

test integrations

elastic-vault-github-plugin-prod · 2024-09-10T08:26:30Z

Created or updated PR in integrations repository to test this version. Check elastic/integrations#11055

jsoriano · 2024-09-10T08:56:20Z

test/packages/parallel/ti_anomali_logsdb/docs/README.md

This change needs to be applied to all packages as found out in https://buildkite.com/elastic/integrations/builds/15696

This makes the application of this change quite inconvenient. We would need to find some mechanism that removes this line only on new files or when explicitly regenerating. Do you think this could be an option?

ISTM that a separate PR in integrations that does

parallel "perl -p -i -e 'chomp if eof'" {} ::: $(rg -l -U -g 'sample_event.json' '^}\n') for d in $(git diff --name-only|cut -f1,2 -d/|sort|uniq); do (cd $d; elastic-package build); done

would fix all the cases to pass in both versions of elastic-package. This would be done prior to bumping the ep version for integrations.

would fix all the cases to pass in both versions of elastic-package. This would be done prior to bumping the ep version for integrations.

Would this be compatible with the current version of elastic-package?

Yes, that mutation takes the state of the packages to what they would be without human intervention.

I mean, this change breaks linting on current integrations, and applying the fix in current integrations with current elastic-package will also break linting.

We should find a way of keeping compatibility with current integrations. When we needed to do this kind of changes we have done them in a way that depend on a spec version, so updating the spec version is what triggers the new behaviour. Or we have kept compatibility with both formats.

This is the change elastic/integrations#11174.

Do we need to have changelog entries for packages if we are just fixing these test defects?

No need for this (unless some team wants this change to be published).

This is the change elastic/integrations#11174.

Thanks, this helps.

Looking at the needed changes, what would you think about applying the fix in elastic-package only to the methods that write the events, and not to renderSampleEvent? This way we would keep compatibility with new and old packages.

Looking at the needed changes, what would you think about applying the fix in elastic-package only to the methods that write the events, and not to renderSampleEvent? This way we would keep compatibility with new and old packages.

No, this doesn't make sense as we are adding the newline in one place and removing in the other.

What I think confuses me from elastic/integrations#11174 is that we are removing the newlines, but here we are adding them, looks like doing the opposite of what we are intending here.

This is code repair. The ultimate goal is to have a state of code that is agnostic to the presence of final new lines in the sample_event.json files. This does that. The change in elastic/integrations#11174 works around (by fixing the packages to conform exactly to what ep emits) the weaknesses in the linting code in ep. The new code here does not care about whether the sample_event.json file has a final new line, it will always render the event with a final newline, and when rendering it remove any trailing whitespace and add the necessary newline when required.

The deeper background for this (and some of the correctness issues) can be see here.

jsoriano

I think we could make the change in docs rendering dependant on the spec version, this way we don't need to do any change in packages now, and it will be enforced as packages upgrade the format versions they use. We already do this in other cases.

jsoriano · 2024-09-19T14:15:59Z

internal/docs/sample_event.go

 	}
 	builder.WriteString("```json\n")
-	builder.Write(formatted)
+	builder.Write(bytes.TrimSpace(formatted))


To avoid having different results in existing packages, wdyt about making this dependant on the spec version? This way the change in packages is controlled by a change in the spec. This is already done for other cases now.

Suggested change

builder.Write(bytes.TrimSpace(formatted))

if !specVersion.LessThan(semver.MustParse("3.2.3")) {

builder.Write(bytes.TrimSpace(formatted))

}

This is not necessary.

How can we make this change compatible with existing packages then?

Even if we merge elastic/integrations#11174, older packages, and older versions of elastic-package will be incompatible.

It is already compatible, if the package conforms to the ep generated format. This is the point of the preparatory change. Once we get everything into the canonical form, the two states of this code (this change and the previous version) both produce the same README files and so the checks will pass. The sample files will differ, but they are not checked, which is essentially why the preparatory change is required.

jsoriano · 2024-09-19T14:20:11Z

test/packages/parallel/ti_anomali_logsdb/docs/README.md

What I think confuses me from elastic/integrations#11174 is that we are removing the newlines, but here we are adding them, looks like doing the opposite of what we are intending here.

jsoriano

Discussed over slack, let's go with this change but please merge elastic/integrations#11174 first.

efd6 · 2024-09-20T08:15:45Z

That is the intention.

efd6 · 2024-09-20T08:38:35Z

Converted to draft to avoid accidental merge.

jsoriano · 2024-09-24T08:36:11Z

test integrations

elastic-vault-github-plugin-prod · 2024-09-24T08:42:30Z

Created or updated PR in integrations repository to test this version. Check elastic/integrations#11055

efd6 · 2024-09-30T05:23:51Z

test integrations

elastic-vault-github-plugin-prod · 2024-09-30T05:34:03Z

Created or updated PR in integrations repository to test this version. Check elastic/integrations#11055

efd6 · 2024-09-30T21:20:16Z

@jsoriano The test failures in the test for this are unrelated to lint checks, so I think this is good to merge. PTAL

efd6 self-assigned this Sep 6, 2024

internal/{docs,testrunner/runners/system,internal/testrunner/runners/…

ff78679

…pipeline}: make json files end with newlines

efd6 force-pushed the final_newlines branch from 04157c7 to ff78679 Compare September 8, 2024 20:44

efd6 added the enhancement New feature or request label Sep 8, 2024

efd6 mentioned this pull request Sep 8, 2024

[tychon] New integration elastic/integrations#10811

Merged

efd6 marked this pull request as ready for review September 8, 2024 21:38

chrisberkhout reviewed Sep 9, 2024

View reviewed changes

elastic-vault-github-plugin-prod bot mentioned this pull request Sep 10, 2024

Test elastic-package#2087 - DO NOT MERGE elastic/integrations#11055

Closed

jsoriano reviewed Sep 10, 2024

View reviewed changes

efd6 mentioned this pull request Sep 18, 2024

all: fix sample_event.json final newlines elastic/integrations#11174

Merged

4 tasks

jsoriano reviewed Sep 19, 2024

View reviewed changes

jsoriano approved these changes Sep 20, 2024

View reviewed changes

efd6 marked this pull request as draft September 20, 2024 08:38

efd6 marked this pull request as ready for review September 30, 2024 21:20

jsoriano merged commit 74eb18f into elastic:main Oct 1, 2024

	func JSONFormatterBuilder(specVersion semver.Version) JSONFormatter {
	if specVersion.LessThan(semver.MustParse("2.12.0")) {
	return &jsonFormatterWithHTMLEncoding{}
	}

	return &jsonFormatter{}
	}

	// jsonFormatterWithHTMLEncoding function is responsible for formatting the given JSON input.
	// It encodes special HTML characters.
	type jsonFormatterWithHTMLEncoding struct{}

	func (jsonFormatterWithHTMLEncoding) Format(content []byte) ([]byte, bool, error) {
	var rawMessage json.RawMessage
	err := json.Unmarshal(content, &rawMessage)
	if err != nil {
	return nil, false, fmt.Errorf("unmarshalling JSON file failed: %w", err)
	}

	formatted, err := json.MarshalIndent(&rawMessage, "", " ")
	if err != nil {
	return nil, false, fmt.Errorf("marshalling JSON raw message failed: %w", err)
	}
	return formatted, string(content) == string(formatted), nil
	}

	func (jsonFormatterWithHTMLEncoding) Encode(doc any) ([]byte, error) {
	return json.MarshalIndent(doc, "", " ")
	}

	// jsonFormatter function is responsible for formatting the given JSON input.
	type jsonFormatter struct{}

	func (jsonFormatter) Format(content []byte) ([]byte, bool, error) {
	var formatted bytes.Buffer
	err := json.Indent(&formatted, content, "", " ")
	if err != nil {
	return nil, false, fmt.Errorf("formatting JSON document failed: %w", err)
	}

	return formatted.Bytes(), bytes.Equal(content, formatted.Bytes()), nil
	}

	func (jsonFormatter) Encode(doc any) ([]byte, error) {
	var formatted bytes.Buffer
	enc := json.NewEncoder(&formatted)
	enc.SetEscapeHTML(false)
	enc.SetIndent("", " ")

	err := enc.Encode(doc)
	if err != nil {
	return nil, err
	}

	// Trimming to be consistent with MarshalIndent, that seems to trim the result.
	return bytes.TrimSpace(formatted.Bytes()), nil
	}

Conversation

efd6 commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Build Failed

Failed CI Steps

History

Uh oh!

chrisberkhout left a comment

Choose a reason for hiding this comment

Uh oh!

efd6 commented Sep 9, 2024

Uh oh!

jsoriano commented Sep 10, 2024

Uh oh!

jsoriano commented Sep 10, 2024

Uh oh!

elastic-vault-github-plugin-prod bot commented Sep 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efd6 Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

efd6 commented Sep 20, 2024

Uh oh!

efd6 commented Sep 20, 2024

Uh oh!

jsoriano commented Sep 24, 2024

Uh oh!

elastic-vault-github-plugin-prod bot commented Sep 24, 2024

Uh oh!

efd6 commented Sep 30, 2024

Uh oh!

elastic-vault-github-plugin-prod bot commented Sep 30, 2024

Uh oh!

efd6 commented Sep 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

efd6 commented Sep 6, 2024 •

edited

Loading

elasticmachine commented Sep 6, 2024 •

edited

Loading

efd6 Sep 19, 2024 •

edited

Loading

jsoriano left a comment •

edited

Loading