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

fix(core): improve config management package #233

Closed
wants to merge 1 commit into from

Conversation

namit-chandwani
Copy link
Member

WIP (Up for an early discussion)

Closes #184

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation change
  • Other (please specify)

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer

@namit-chandwani namit-chandwani added the WIP work in progress label Aug 17, 2021
@@ -0,0 +1,100 @@
package newconfig
Copy link
Contributor

Choose a reason for hiding this comment

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

new?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just did it temporarily so that you can notice the changes easily.
Will replace it with the old config package once everything here is done

type Config struct {
}

type IConfig interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Too generic name

return nil
}

func getJsonSubkey(data []byte, keyName string) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Member Author

Choose a reason for hiding this comment

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

return []byte(val), nil
}

func deleteJsonSubkey(data []byte, keyName string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused

Copy link
Member Author

Choose a reason for hiding this comment

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

return nil
}

func (h *hostCfgHandler) Remove() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of remove?

@wtrocki
Copy link
Contributor

wtrocki commented Aug 17, 2021

@namit-chandwani I do not see how this would work.

Approach with subkeys is not going to allow us to load and write structure. Please look at example plugin and example host. Modify them to use different configs and we can discuss later hoe to swap the config from host. Proposed PR doesn't differ much with config on main branch that we covered over last call.

@wtrocki wtrocki closed this Aug 17, 2021
@wtrocki wtrocki deleted the fix/core/improve-config branch August 17, 2021 20:06
@wtrocki
Copy link
Contributor

wtrocki commented Aug 17, 2021

Ignore comments. Lets work with @ankithans to help him with examples and solve config outside core first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core config feature is not working as expected.
2 participants