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.

Sign up for GitHub

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

Jump to bottom

feat(addons): add maintenance windows manipulation with the new addons-config command #955

Merged
merged 19 commits into from
Jul 13, 2023
Merged

feat(addons): add maintenance windows manipulation with the new addons-config command #955

merged 19 commits into from
Jul 13, 2023

Conversation

SCedricThomas
Copy link
Contributor

@SCedricThomas SCedricThomas commented May 30, 2023 •
edited
Loading

fix #952

  • Add a changelog entry in the section "To Be Released" of CHANGELOG.md

@SCedricThomas SCedricThomas self-assigned this May 30, 2023
@SCedricThomas SCedricThomas changed the title feat(maintenance-window): implement addon-config Maintenance windows manipulation May 30, 2023
@SCedricThomas
Copy link
Contributor Author

We will need a new release of go-scalingo in order to pass the CI but can you take a look at my code ?
You can take a look to the specifications here.
Here is my output:
image

EtienneM
EtienneM requested changes May 31, 2023
View reviewed changes
Copy link
Member

@EtienneM EtienneM left a comment

Choose a reason for hiding this comment

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

The failed CI and the release of go-scalingo are 2 completely different things. I don't understand why there is a link between these two?

addons/config.go Outdated Show resolved Hide resolved
addons/info.go Outdated Show resolved Hide resolved
cmd/addons.go Outdated Show resolved Hide resolved
cmd/addons.go Outdated Show resolved Hide resolved
cmd/addons.go Show resolved Hide resolved
addons/config.go Outdated Show resolved Hide resolved
addons/config.go Outdated Show resolved Hide resolved
addons/config.go Outdated Show resolved Hide resolved
addons/config.go Outdated Show resolved Hide resolved
addons/config.go Outdated Show resolved Hide resolved
@SCedricThomas
Copy link
Contributor Author

SCedricThomas commented May 31, 2023 •
edited
Loading

The failed CI and the release of go-scalingo are 2 completely different things. I don't understand why there is a link between these two?

I assumed that the CI build will fail because I locally linked go-scalingo to master without a proper release

@EtienneM EtienneM changed the title Maintenance windows manipulation feat(addons): add maintenance windows manipulation with the new addons-config command Jun 2, 2023
EtienneM
EtienneM approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@EtienneM EtienneM left a comment

Choose a reason for hiding this comment

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

I'm already approving the PR but I would love if you could:

  • take into account the remaining comments
  • release a new go-scalingo version and use it here

CHANGELOG.md Outdated Show resolved Hide resolved
cmd/addons.go Outdated
errorQuitWithHelpMessage(errors.New(ctx, "cannot update your addon without a specified option"), c, "addons-config")
}

err := addons.UpdateAddonConfig(ctx, currentApp, currentAddon, config)
Copy link
Member

Choose a reason for hiding this comment

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

best practice: I'll be a bit picky on this one, there is a linter which should raise an error here. It's not useful to add the package name in the method name. Hence:

Suggested change
err := addons.UpdateAddonConfig(ctx, currentApp, currentAddon, config)
err := addons.UpdateConfig(ctx, currentApp, currentAddon, config)

@SCedricThomas SCedricThomas marked this pull request as ready for review July 6, 2023 08:16
@SCedricThomas SCedricThomas marked this pull request as draft July 6, 2023 08:16
SCedricThomas and others added 17 commits July 12, 2023 16:39
@SCedricThomas @curzolapierre
feat(maintenance-window): implement addon-config
ac293eb
@SCedricThomas @curzolapierre
chore: comment
d4a342f
@SCedricThomas @curzolapierre
feat: maintenance window validation
3926976
@SCedricThomas @curzolapierre
chore: coding style
b490dec
@SCedricThomas @curzolapierre
chore(CHANGELOG): add a new changelog entry for the current PR
1557e38
@SCedricThomas @EtienneM @curzolapierre
Update addons/info.go
f56c1c0 
Co-authored-by: Étienne M. <[email protected]>
@SCedricThomas @EtienneM @curzolapierre
Update addons/config.go
5f8e780 
Co-authored-by: Étienne M. <[email protected]>
@SCedricThomas @EtienneM @curzolapierre
Update addons/config.go
6a951c0 
Co-authored-by: Étienne M. <[email protected]>
@SCedricThomas @EtienneM @curzolapierre
Update addons/config.go
1d912be 
Co-authored-by: Étienne M. <[email protected]>
@SCedricThomas @EtienneM @curzolapierre
Update addons/config.go
f4577a3 
Co-authored-by: Étienne M. <[email protected]>
@SCedricThomas @curzolapierre
chore: improve code quality
cf01240
@SCedricThomas @curzolapierre
Revert "chore: coding style"
3254a5b 
This reverts commit 7c97302.
@SCedricThomas @curzolapierre
chore: linter
9d2e349
@SCedricThomas @EtienneM @curzolapierre
Update CHANGELOG.md
415b97b 
Co-authored-by: Étienne M. <[email protected]>
@SCedricThomas @curzolapierre
fix: rename addons package update method
78a7bd7
@curzolapierre
deps: update go-scalingo from v6.6.0 to v6.7.1
96f37b1
@curzolapierre
chore: rebase on master
d60432f
@curzolapierre curzolapierre marked this pull request as ready for review July 12, 2023 16:31
curzolapierre
curzolapierre reviewed Jul 13, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@SCedricThomas @curzolapierre
Update CHANGELOG.md
0de41f6 
Co-authored-by: Pierre Curzola <[email protected]>
curzolapierre
curzolapierre reviewed Jul 13, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@SCedricThomas @curzolapierre
Update CHANGELOG.md
f323870 
Co-authored-by: Pierre Curzola <[email protected]>
yohann-bacha
yohann-bacha approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@yohann-bacha yohann-bacha left a comment

Choose a reason for hiding this comment

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

great job! but we should be sure that the utc conversions are done

@SCedricThomas SCedricThomas merged commit f20a49a into master Jul 13, 2023
@SCedricThomas SCedricThomas deleted the feat/952/maintenance-windows branch July 13, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@curzolapierre curzolapierre curzolapierre left review comments

@yohann-bacha yohann-bacha yohann-bacha approved these changes

@EtienneM EtienneM EtienneM approved these changes

Assignees

@SCedricThomas SCedricThomas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[cli] Maintenance windows should be visible/editable
4 participants
@SCedricThomas @EtienneM @curzolapierre @yohann-bacha