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

Fail when cluster commands are called without a proper workspace set #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

diogob
Copy link
Collaborator

@diogob diogob commented Jan 13, 2025

When a cluster command such as migrate or start is called without a proper configuration the current CLI proceeds with an empty config that yields behavior hard to understand for a newcomer.

Proposed solution

We can be more strict requiring an existing config to run such commands.
This also allows for better error messages guiding the user during the initial project creation.

@@ -41,6 +43,10 @@ func migrate(ctx context.Context, cluster orb.OrbCluster, dbReset bool, orbs []s
err = errors.New("orbs and databases have to be of the same size")
return
}
if len(orbs) == 0 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this change is a stowaway here, just was testing and thought it would be nice to cover this case even if remote.

@diogob diogob marked this pull request as ready for review January 13, 2025 18:59
@@ -62,12 +62,8 @@ func LoadConfig(path string) (cfg *Config, err error) {
v.SetConfigFile(filepath.Join(path, "omnigres.yaml"))
err = v.ReadInConfig()
if err != nil {
if _, ok := err.(*os.PathError); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change may affect omnigres run as it can run files from a directory without a yaml file – so handling the error there might be needed.

This assumes we agree that omnigres run should be able to run yaml-less directories (by just throwing in a default config as the removed code did) – so we'll need to handle it there, in run.go – if the file is not found (error is of a certain type denoting that) then supply NewConfig()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was actually dropping that behavior (running on yaml-less dirs) for the sake of simplicity. I thought run would be analogous to start in that sense.
Should we keep this behavior for the run command?

orb/config.go Outdated
return
}
return
msg := fmt.Sprintf("omnigres.yaml not found in %s. Try setting a different workspace with -w or running omnigres init to create a new project config.", path)
Copy link
Contributor

Choose a reason for hiding this comment

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

For the idea above we'd need to type the error.

But outside of typing, there's fmt.Errorf that accomplishes errors.New(fmt.Sprintf())

@diogob diogob force-pushed the require-config-when-running-cluster-cmds branch from 20fb47d to ef710e8 Compare January 14, 2025 14:13
without a proper configuration the current CLI proceeds with an empty
config that yields behavior hard to understand for a newcomer.

Solution: We can be more strict requiring an existing config to run such
commands. This also allows for better error messages guiding the user
during the initial project creation.
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.

2 participants