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

Support configuration file for export command #55

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

WordlessEcho
Copy link

@WordlessEcho WordlessEcho commented Mar 29, 2023

Resolves #54

If both command parameters and configuration file is specified. It will respect the options of command parameters.
Improvement: Maybe we should make default value of command parameters as constant?

Tested on my environment by following configurations:
~/config.json

{
    "outputDir": "~/export-test",
    "channels": ["t"]
}
go build
./inter-server-sync export --config=~/config.json --channels=testchannel

Result:

$ cat ~/export-test/exportedChannels.txt
testchannel

- `options.ServerConfig`: /etc/rhn/rhn.conf
- `options.OutputFolder`: .
Use utils/utils.GetAbsPath to get path
The path of version.txt can be modified by SetOptionsByConfig()
@WordlessEcho
Copy link
Author

About 5be4e6d : ServerConfig is belongs to cmd/root.go. I think we should not change it in cmd/export.go?

@avshiliaev avshiliaev self-requested a review March 31, 2023 14:18
Copy link
Contributor

@avshiliaev avshiliaev left a comment

Choose a reason for hiding this comment

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

I like your proposal, we could definitely improve it a bit and include into our project if you'd like top work on it.

cmd/export.go Outdated
@@ -27,6 +28,7 @@ var includeContainers bool
var orgs []uint

func init() {
exportCmd.Flags().StringVar(&config, "config", "", "Location of configuration file")
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a nice approach. I'd like it to be a bit more explicit on the user interface level, that we for example can either provide a config or specify it as a set of args. This way we could probably not mix them together with the options. What do you think?

Copy link
Author

@WordlessEcho WordlessEcho Apr 4, 2023

Choose a reason for hiding this comment

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

@avshiliaev Sorry. I didn't catch your point... Can you give me some examples?
Or I guess that you hope the --config could be global flag as same as --serverConfig?

entityDumper/types.go Outdated Show resolved Hide resolved
entityDumper/exportConfigDumper.go Outdated Show resolved Hide resolved
@WordlessEcho WordlessEcho marked this pull request as draft April 5, 2023 10:42
@WordlessEcho
Copy link
Author

I found some bug in recent commits. Waiting for fix.

Parse []uint by json.Unmarshal
spf13/viper#926
@WordlessEcho WordlessEcho marked this pull request as ready for review April 5, 2023 13:51
@WordlessEcho
Copy link
Author

WordlessEcho commented Apr 5, 2023

Bug fixed.
About the --config option: I wrote some codes to move it into cmd/root.go as global flag but not pushed yet.

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.

[feature request] support configuration file
2 participants