feat(serve): custom config option #761

lumtis · 2021-02-16T13:30:53Z

-c/--config specifies a custom starport config file for the serve command

lumtis · 2021-02-16T16:34:05Z

Provide only the file name instead of the file path. This is more convenient because fswatcher has the path of the project as a workdir and we must use it to watch for config changes

ilgooz · 2021-02-17T06:08:59Z

starport/services/chain/chain.go

+// ConfigName specifies a custom config file to use
+func ConfigName(configName string) Option {
+	return func(c *Chain) {
+		c.options.ConfigName = configName


Maybe we can refactor LocateDefault func to Locate(root string, []string) where []string is usually set as conf.FileNames but when ConfigName option is used it is set as []string{configName}. This might simplify some parts in the code.

LocateDefault has a different purpose, it has a list of file that can be considered as the default configuration then if we find nothing we can properly handle the error to say that we use a default hard coded configuration.

Custom option responds to a different logic, you always have a single file and if you don't find it you always return an error.

I think it's more maintainable to separate the two if we want to change the logic to search for the default configuration in the future

ilgooz · 2021-02-17T06:15:00Z

Provide only the file name instead of the file path. This is more convenient because fswatcher has the path of the project as a workdir and we must use it to watch for config changes

I think it is kind of expected to be able to provide a relative/abs path when setting a config path flag in a CLI. I beleive it is better to stick with this convention.

lumtis · 2021-02-17T07:41:35Z

Provide only the file name instead of the file path. This is more convenient because fswatcher has the path of the project as a workdir and we must use it to watch for config changes

I think it is kind of expected to be able to provide a relative/abs path when setting a config path flag in a CLI. I beleive it is better to stick with this convention.

I try with filepath.Rel for the fswatcher issue. If this doesn't work and need a more complex solution I would prefer to rename the option --config-name and track the issue since we initially need this feature to test IBC applications

ilgooz · 2021-02-17T07:48:32Z

We can first run path := filepath.Abs(custumConfigRelOrAbs) and check if filepath.IsAbs(path) before joining paths in fswatcher, if it is, we don't need to join them.

lumtis · 2021-02-17T07:56:03Z

Nevermind I didn't notice this logic is from our side. It's easy to fix then

ilgooz

Btw, can we avoid updating starport/ui/dist-go/statik/statik.go in case it contains an older version of the ui?

lumtis · 2021-02-17T09:03:07Z

starport/ui/dist-go/statik/statik.go is automatically updated when running gofmt do you think it's a problem?

[EDIT] added ui to skipped dir for golint so no more problem

fadeev

Works with a custom foo.yml config file.

ilgooz

Asked for some tweaks. 👍

starport/interface/cli/starport/cmd/serve.go

starport/pkg/fswatcher/fswatcher.go

ilgooz · 2021-02-17T16:35:29Z

starport/services/chain/serve.go

 	// make sure that config.yml exists.
-	if _, err := conf.Locate(c.app.Path); err != nil {
+	if c.options.ConfigFile != "" {
+		if _, err := os.Stat(c.options.ConfigFile); err != nil {


With #761 (review) we don't need a change in this code block as well. It can just call conf.Locate(root, c.options.ConfigFiles).

ilgooz · 2021-02-17T16:36:38Z

starport/services/chain/chain.go

 }
+
+// AppBackendConfigWatchPaths returns the files to watch for config changes
+func (c *Chain) AppBackendConfigWatchPaths() []string {


#761 (review) can eliminate this func as well.

ilgooz · 2021-02-17T16:36:56Z

starport/services/chain/chain.go

 func (c *Chain) Config() (conf.Config, error) {
-	path, err := conf.Locate(c.app.Path)
+	if c.options.ConfigFile != "" {
+		return conf.ParseFile(c.options.ConfigFile)


#761 (review) can eliminate this change as well.

ilgooz · 2021-02-17T16:38:10Z

starport/services/chain/chain.go

+// ConfigFile specifies a custom config file to use
+func ConfigFile(configFile string) Option {
+	return func(c *Chain) {
+		c.options.ConfigFile = configFile


Regarding to #761 (review), c.options.ConfigFile can be conf.FileNames by default and call to ConfigFile with a non-empty string will change it to []string{configFile}.

I think it's a bit ambiguous. The option should be a single string, conf.FileNames is an array because you want to have several possibilities of the file name for the default config

ilgooz · 2021-02-17T16:39:44Z

starport/interface/cli/starport/cmd/serve.go

+		return err
+	}
+	if config != "" {
+		chainOption = append(chainOption, chain.ConfigFile(os.ExpandEnv(config)))


We can I think, pass this option anyway even if config path is an empty string but check for an empty string inside chain.ConfigFile to fallback to conf.FileNames.

Mmmh then we don't use the benefit of this pattern which is to have optional parameters?
Or else we use a positional argument

lumtis · 2021-02-17T21:34:01Z

Thank for the review I check this.
The only problem in the solution is we currently use the default hardcoded config if config.yml is not found while we should return an error if the provided config doesn't exist

fadeev

Looking good!

ilgooz

👍

* Custom config * Custom config option * Lint * Add test for custom config file * Change option name * SOme naming * Fix test * refacto: specify file name * Lint * Suport absolute paths * Revert statik.go * Lint * Add directory skip for lint * Ilker suggestions * Verifyu config exists * COnfigPath * No config check

lumtis added 4 commits February 16, 2021 13:47

Custom config

19a4694

Custom config option

5101bd7

Lint

1ceaae9

Add test for custom config file

f897375

lumtis linked an issue Feb 16, 2021 that may be closed by this pull request

feat(serve): adding -c/--config flag #759

Closed

lumtis added 5 commits February 16, 2021 14:31

Change option name

17135b8

SOme naming

f1f0d4a

Fix test

1a95d9a

refacto: specify file name

1442121

Lint

20e8824

lumtis marked this pull request as ready for review February 16, 2021 16:31

lumtis requested review from fadeev and ilgooz as code owners February 16, 2021 16:31

ilgooz reviewed Feb 17, 2021

View reviewed changes

lumtis added 3 commits February 17, 2021 09:36

Suport absolute paths

8b3f02d

Revert statik.go

a95a936

Lint

902a998

lumtis requested a review from ilgooz February 17, 2021 09:03

Add directory skip for lint

faf6ab5

fadeev previously approved these changes Feb 17, 2021

View reviewed changes

ilgooz requested changes Feb 17, 2021

View reviewed changes

starport/interface/cli/starport/cmd/serve.go Outdated Show resolved Hide resolved

starport/pkg/fswatcher/fswatcher.go Outdated Show resolved Hide resolved

ilgooz reviewed Feb 17, 2021

View reviewed changes

Ilker suggestions

ca0cfa5

lumtis dismissed fadeev’s stale review via ca0cfa5 February 17, 2021 16:48

lumtis added 3 commits February 18, 2021 09:20

Verifyu config exists

c7e4ef0

COnfigPath

5a06d66

No config check

6429ca2

lumtis requested review from fadeev and ilgooz February 18, 2021 10:07

fadeev approved these changes Feb 18, 2021

View reviewed changes

ilgooz approved these changes Feb 18, 2021

View reviewed changes

ilgooz merged commit c97bd6c into develop Feb 18, 2021

ilgooz deleted the feat/config-option branch February 18, 2021 12:09

feat(serve): custom config option #761

feat(serve): custom config option #761

Uh oh!

Conversation

lumtis commented Feb 16, 2021

Uh oh!

lumtis commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilgooz commented Feb 17, 2021

Uh oh!

lumtis commented Feb 17, 2021

Uh oh!

ilgooz commented Feb 17, 2021

Uh oh!

lumtis commented Feb 17, 2021

Uh oh!

ilgooz left a comment

Choose a reason for hiding this comment

Uh oh!

lumtis commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fadeev left a comment

Choose a reason for hiding this comment

Uh oh!

ilgooz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ilgooz Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lumtis commented Feb 17, 2021

Uh oh!

fadeev left a comment

Choose a reason for hiding this comment

Uh oh!

ilgooz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lumtis commented Feb 17, 2021 •

edited

Loading

ilgooz Feb 17, 2021 •

edited

Loading