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

Added Update and Replace APIs for Service custom values #2522

Merged
merged 11 commits into from
Sep 1, 2023
Merged

Added Update and Replace APIs for Service custom values #2522

merged 11 commits into from
Sep 1, 2023

Conversation

andreas-kupries
Copy link
Contributor

@andreas-kupries andreas-kupries commented Aug 25, 2023 •
edited

fix #2518

@andreas-kupries andreas-kupries added the kind/bug Something isn't working label Aug 25, 2023
@andreas-kupries andreas-kupries added this to the v1.10.0 milestone Aug 25, 2023
@andreas-kupries andreas-kupries self-assigned this Aug 25, 2023
@andreas-kupries
Copy link
Contributor Author

confirmed breakage in local CI run. investigate after the weekend

@codecov
Copy link

codecov bot commented Aug 28, 2023 •
edited

Codecov Report

Patch coverage is 47.23% of modified lines.

Files Changed Coverage
internal/api/v1/router.go ø
internal/api/v1/service/replace.go 0.00%
internal/cli/usercmd/client.go ø
pkg/api/core/v1/models/models.go ø
internal/api/v1/configuration/replace.go 25.00%
internal/api/v1/configuration/update.go 33.33%
internal/services/instances.go 36.80%
internal/api/v1/service/update.go 47.91%
internal/cli/services.go 51.61%
internal/cli/configuration.go 56.25%
... and 6 more

📢 Thoughts on this report? Let us know!.

@andreas-kupries andreas-kupries marked this pull request as ready for review August 29, 2023 10:29
@andreas-kupries andreas-kupries requested a review from a team as a code owner August 29, 2023 10:29
@andreas-kupries
Copy link
Contributor Author

@richard-cox For the UI we have a new Replace API endpoint, analogous to what the server offers for configurations. The CLI uses the new Update endpoint.

@richard-cox richard-cox mentioned this pull request Aug 29, 2023
Closed
@enrichman
Copy link
Member

There are some conflicts to resolve. One first thing is that I would change the --remove flag to --unset, to have the --set/--unset pair available.

@andreas-kupries
Copy link
Contributor Author

Will resolve the conflicts.

@andreas-kupries
Copy link
Contributor Author

One first thing is that I would change the --remove flag to --unset, to have the --set/--unset pair available.

Note that making this change would make this different from the configuration update command which uses the same --remove/--set pair.

@andreas-kupries
fixes in semi-related parts of the system
545f964 
- removed redundant check
- added documentation of Update/Replace for configurations.
- fixed comment typo
@andreas-kupries
chore: refactored code (app restart after change) common to config up…
eaff7e8 
…date/replace into singular function.

note: this will be used by service update/replace too.
@andreas-kupries
chore: moved service structures to separate file
a324f32
@andreas-kupries
added update/replace models for services, analogous to configs.
225c8a6 
further reordered the types to separate catalog from service.
@andreas-kupries
refactor create api endpoint, backend somewhat
4a5d9f4 
- moved post hook into adressable function, leaving only the closure
- moved deploy code into separate function for use by coming update/replace
@andreas-kupries
backend for update/replace service data
9f16a76
@andreas-kupries
api endpoints for service update/replace
15e72eb
@andreas-kupries
place display of change instructions in separate function
cd99ec6
@andreas-kupries
service update command, logic, client backend
facffda
@andreas-kupries
tests, service update command
06deedc
@andreas-kupries
Copy link
Contributor Author

One first thing is that I would change the --remove flag to --unset, to have the --set/--unset pair available.

Note that making this change would make this different from the configuration update command which uses the same --remove/--set pair.

Finding https://github.com/spf13/pflag#mutating-or-normalizing-flag-names I will see if I can support the old --remove while switching to --unset going forward. That way we would have backward compatibility.

@andreas-kupries
Copy link
Contributor Author

One first thing is that I would change the --remove flag to --unset, to have the --set/--unset pair available.

Note that making this change would make this different from the configuration update command which uses the same --remove/--set pair.

Finding https://github.com/spf13/pflag#mutating-or-normalizing-flag-names I will see if I can support the old --remove while switching to --unset going forward. That way we would have backward compatibility.

The normalizing stuff as shown was not working out. It also affects the help texts (because the flag name in the help is the normalization result, so you see the new name, together with old short hand and help text).

Done in a different way with commit 10f3921

@andreas-kupries andreas-kupries merged commit f3b6abd into main Sep 1, 2023
18 checks passed
@andreas-kupries andreas-kupries deleted the 2518-service-instance-edit-cv branch September 1, 2023 10:58
@richard-cox richard-cox mentioned this pull request Sep 5, 2023
Closed
@andreas-kupries andreas-kupries mentioned this pull request Sep 8, 2023
Merged
@enrichman enrichman added kind/enhancement New feature or request and removed kind/bug Something isn't working labels Sep 25, 2023
@enrichman enrichman changed the title fixed missing support for edit of service (custom values, update/replace) Added Update and Replace APIs for Service custom values Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enrichman enrichman enrichman approved these changes

Assignees

@andreas-kupries andreas-kupries

Labels
kind/enhancement New feature or request
Projects
Epinio planning
Archived in project
Milestone
v1.10.0
Development

Successfully merging this pull request may close these issues.

It's not possible to edit Service chart values/settings
2 participants
@andreas-kupries @enrichman