Skip to content

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
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?

@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