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

Add pause resume #413

Merged
merged 11 commits into from
Apr 26, 2024
Merged

Add pause resume #413

merged 11 commits into from
Apr 26, 2024

Conversation

olevitt
Copy link
Contributor

@olevitt olevitt commented Apr 9, 2024

Implementing solution 1 from InseeFrLab/onyxia#791
Still needs to :

  • Test, refine, cleanup
  • Handle failures / non-200 cases properly
  • Return the status (paused / not paused) when returning services
  • Add events

@github-actions github-actions bot added the helm-wrapper Related to the helm-wrapper library label Apr 9, 2024
Comment on lines +414 to +422
Region region,
Project project,
String catalogId,
Pkg pkg,
User user,
String serviceId,
boolean skipTlsVerify,
String caFile,
boolean dryRun)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of duplicating these parameteres for so many methods, what do you think about packing these parameters into an DTO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would make a lot of sense. Got a headache yesterday working on parameters overloading 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Another suggestion could be to have one method taking in a status, instead of having two methods (pause and resume).
If that make sense?
E.g.: public void update(Region region, ... , Status status where status is an enum (PAUSE,RESUME), i.e.. Maybe it could be possible to further refactor such that delete also is a part of that enum.
What do you think?

Copy link

sonarcloud bot commented Apr 17, 2024

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ihiverlet ihiverlet marked this pull request as ready for review April 26, 2024 07:58
@ihiverlet ihiverlet merged commit 8765544 into main Apr 26, 2024
8 checks passed
@ihiverlet ihiverlet deleted the add-pause-resume branch April 26, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-wrapper Related to the helm-wrapper library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants