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

feat: Add json_timestamp_layout option #8229

Merged
merged 2 commits into from
Sep 21, 2021
Merged

feat: Add json_timestamp_layout option #8229

merged 2 commits into from
Sep 21, 2021

Conversation

HeikoSchlittermann
Copy link
Contributor

@HeikoSchlittermann HeikoSchlittermann commented Oct 6, 2020 •
edited
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@HeikoSchlittermann
Copy link
Contributor Author

Is there any chance to get this PR merged into the upstream sources? It would greatly improve the chance for the integration of telegraf into a legacy environment.

@sjwang90 sjwang90 added area/json json and json_v2 parser/serialiser related feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 23, 2020
sspaink
sspaink suggested changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request, added two comments for you to review. Overall it looks good I think. I know its been a while since you've made this pr, are you still using this change?

plugins/serializers/json/README.md Outdated Show resolved Hide resolved
plugins/serializers/json/json.go Outdated Show resolved Hide resolved
@HeikoSchlittermann HeikoSchlittermann changed the title Provide json_timestamp_layout option ready to merge: Provide json_timestamp_layout option Aug 6, 2021
@HeikoSchlittermann HeikoSchlittermann changed the title ready to merge: Provide json_timestamp_layout option feat: Provide json_timestamp_layout option Aug 6, 2021
@HeikoSchlittermann HeikoSchlittermann changed the title feat: Provide json_timestamp_layout option feat: Add json_timestamp_layout option Aug 6, 2021
@HeikoSchlittermann
Copy link
Contributor Author

HeikoSchlittermann commented Aug 6, 2021 •
edited
Loading

Please apply this before #8184. I got a circular dependency in the unit tests, which I can fix as soon as this (#8229) is merged into master.

Otherwise this and #8184 are independent on each other.

@HeikoSchlittermann HeikoSchlittermann mentioned this pull request Aug 6, 2021
Merged
3 tasks
sspaink
sspaink approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

lgtm, looks like there are some linter errors that will need to be fixed first before merging.

@sspaink sspaink added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 9, 2021
@HeikoSchlittermann
Copy link
Contributor Author

HeikoSchlittermann commented Aug 10, 2021 •
edited
Loading

The linter errors are outside of this patch's scope, aren't they?

@sspaink
Copy link
Contributor

sspaink commented Aug 10, 2021

The linter error var-naming: func NewJsonSerializer should be NewJSONSerializer is in scope because that function name was changed in this pr.

The other linter error could be considered out of the scope unexported-return but it is just capitalizing the serializer struct, I could do it in a separate pr but then you would have to rebase I assume? Not sure what would be best.

@HeikoSchlittermann
Copy link
Contributor Author

HeikoSchlittermann commented Aug 10, 2021 via email

@HeikoSchlittermann
Copy link
Contributor Author

What's the next step? I'm asking, as, once it is merged, I'd care about my other PR (http unbatched output).

sspaink
sspaink suggested changes Aug 31, 2021
View reviewed changes
Copy link
Contributor

@sspaink sspaink left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Requesting a minor change for linter, then next steps would be someone else on the team needs to review this PR as well and then it can get merged. Thanks!

plugins/serializers/registry.go Outdated Show resolved Hide resolved
plugins/serializers/registry.go Outdated Show resolved Hide resolved
@HeikoSchlittermann
Copy link
Contributor Author

Hello, is there something I can do to get the changes approved?

@popey
Copy link
Contributor

popey commented Sep 14, 2021

Apologies for the delay in getting to this PR. It looks like CI is failing because we've updated the required Go version from 1.16.6 to 1.17 since you made this PR. Please could you rebase off master and once CI passes we'd be happy to merge it. Thanks!

@HeikoSchlittermann
Copy link
Contributor Author

Hi @popey , I rebased (and squashed) my changes onto the head of the current master. Please review again and tell me what changes are necessary to get it upstream.

Thanks and Greetings from Dresden/Germany.

@HeikoSchlittermann
Copy link
Contributor Author

HeikoSchlittermann commented Sep 20, 2021 •
edited
Loading

Right now I changed the commit message to hopefully make the semantic pull request checker happy. And I seem to have succeeded :)

@sspaink: thank you for the approval. Do I need to care that the actual merge will be done?

@MyaLongmire MyaLongmire merged commit b9aa983 into influxdata:master Sep 21, 2021
@HeikoSchlittermann HeikoSchlittermann deleted the hs/json-timestamp-layout branch September 22, 2021 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MyaLongmire MyaLongmire MyaLongmire approved these changes

@sspaink sspaink Awaiting requested review from sspaink

Assignees
No one assigned
Labels
area/json json and json_v2 parser/serialiser related feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HeikoSchlittermann @sspaink @popey @MyaLongmire @sjwang90