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

make cscli use crowdsec version for hub #194

Merged
merged 15 commits into from
Sep 1, 2020

Conversation

AlteredCoder
Copy link
Contributor

No description provided.

Copy link
Contributor

@buixor buixor left a comment

Choose a reason for hiding this comment

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

can we merge the 3 checks for last version + warning display to a single one ? besides that lgtm

cwhub.HubBranch = "master"
} else {
log.Warnf("Crowdsec is not the latest version. Current version is '%s' and latest version is '%s'. Please update it!", cwversion.Version, latest)
cwhub.HubBranch = cwversion.Version
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we want to specify to which branch we're going to stick if it's not master ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is what the ligne above does no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmh my bad, it doesn't precise the branch, but says what is the current crowdsec version and what is the latest. Is it enough ?

/*
if no branch has been specified in flags for the hub, then use the one corresponding to crowdsec version
*/
if cwhub.HubBranch == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above

@@ -63,6 +63,10 @@ you should [update cscli](./cscli_update.md).
if !config.configured {
return fmt.Errorf("you must configure cli before interacting with hub")
}

if err := setHubBranch(); err != nil {
log.Errorf("error while setting hub branch: %s", err)
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 we want to return the error here, so that the PersistentPreRunE can properly fail

Copy link
Contributor

@buixor buixor left a comment

Choose a reason for hiding this comment

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

should return the error in the PersistentPreRunE, so it can fail properly

Copy link
Contributor

@buixor buixor left a comment

Choose a reason for hiding this comment

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

lgtm

@AlteredCoder AlteredCoder merged commit b7286d6 into master Sep 1, 2020
@AlteredCoder AlteredCoder deleted the cscli_use_crowdsec_version_for_hub branch September 1, 2020 12:32
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