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.

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

feat(upgrade): upgrade config #19646

Merged
merged 1 commit into from
Oct 2, 2020

Conversation

alespour
Copy link
Contributor

@alespour alespour commented Sep 25, 2020

This is 3rd PR of 4 for the upgrade feature.
Upgrades config to 2.x format.

It is branched from PR #19641 which should be merged first to avoid conflicts.

@alespour alespour requested a review from rogpeppe September 25, 2020 20:52
@alespour alespour marked this pull request as ready for review September 25, 2020 21:13
@alespour alespour mentioned this pull request Sep 28, 2020
3 tasks
@alespour alespour marked this pull request as draft September 29, 2020 15:46
@alespour alespour removed the request for review from rogpeppe September 29, 2020 16:06
@alespour alespour force-pushed the feat/upgrade-config branch from 80e3d03 to f3286f3 Compare October 1, 2020 08:29
@alespour alespour marked this pull request as ready for review October 1, 2020 09:15
@alespour alespour force-pushed the feat/upgrade-config branch from f3286f3 to 7dd3b91 Compare October 1, 2020 15:25
@stuartcarnie stuartcarnie self-requested a review October 1, 2020 21:46
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Overall this PR looks great and I like the simplicity of a simple mapping that has been used. Great to see tests verifying the transformations 🎉

It is unclear why the rules are encoded in JSON and then embedded using go-bindata and then later unmarshaled. Could you explain the reasoning behind this approach?

@stuartcarnie stuartcarnie added the area/2.x OSS 2.0 related issues and PRs label Oct 1, 2020
@alespour alespour force-pushed the feat/upgrade-config branch from 7dd3b91 to 819b2e3 Compare October 2, 2020 09:23
@alespour alespour force-pushed the feat/upgrade-config branch from 819b2e3 to 225500f Compare October 2, 2020 09:30
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Looks great – thanks for making the improvements 👏

@stuartcarnie stuartcarnie merged commit 1e46509 into influxdata:master Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/2.x OSS 2.0 related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants