-
Notifications
You must be signed in to change notification settings - Fork 24
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
Feat(addons-info): Add addons-info command and documentation #689
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description of your PR should be Fix #578
. With the keyword Fix
, GitHub will automatically close the related issue when merging the PR
You forgot to update the CHANGELOG file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you edit the description of the PR to include an example of execution so that I can see it?
addons/info.go
Outdated
|
||
forceSsl, internetAccess := "disabled", "disabled" | ||
if featuresLen := len(dbInfo.Features); featuresLen > 0 { | ||
if featuresLen == 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really true. There is a possibility that length is greater than 2 and that internet access is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know where I can find the list of possible features for a db ? Thought they were only two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't maintain a comprehensive list of features. Do you have access to appsdeck-database code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so you don't have access to the full list of features.. 😓
CHANGELOG.md
Outdated
@@ -2,6 +2,8 @@ | |||
|
|||
### To be Released | |||
|
|||
* feat(addons-info): add command `addons-info` to display information of an add-on [#578](https://github.com/Scalingo/cli/pull/689) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* feat(addons-info): add command `addons-info` to display information of an add-on [#578](https://github.com/Scalingo/cli/pull/689) | |
* feat(addons-info): add command `addons-info` to display information of an add-on [#689](https://github.com/Scalingo/cli/pull/689) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still this one
addons/info.go
Outdated
forceSsl = strings.ToLower(dbInfo.Features[i]["status"]) | ||
} else if dbInfo.Features[i]["name"] == "publicly-available" { | ||
internetAccess = strings.ToLower(dbInfo.Features[i]["status"]) | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't break in this condition, there is no guarantee of the features order
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there is, since you cannot make your addon publicly available if SSL isn't enforced, so it has to be after the SSL entry. I've checked. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though you supposedly can't enforce Public availability without Force TLS, the order of the features in the array is not guaranteed at all in the code. I can imagine a scenario where this is not the case. For instance, in case of issue with these features, an operator can manually work on the database and the order of features on the database is not guaranteed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I'll remove it then. Thanks for the explanation.
Add
addons-info
command to display addon information :Fix #578