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

Create the MedusaConfiguration API #1180

Merged
merged 5 commits into from
Jan 23, 2024
Merged

Conversation

adejanovski
Copy link
Contributor

What this PR does:
Adds a MedusaConfiguration API.

Which issue(s) this PR fixes:
Fixes #1157

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (ff37de8) 57.19% compared to head (67b213d) 56.92%.

❗ Current head 67b213d differs from pull request most recent head 8af65ea. Consider uploading reports for the commit 8af65ea to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1180      +/-   ##
==========================================
- Coverage   57.19%   56.92%   -0.27%     
==========================================
  Files         101      103       +2     
  Lines       10529    10526       -3     
==========================================
- Hits         6022     5992      -30     
- Misses       3980     4007      +27     
  Partials      527      527              
Files Coverage Δ
...ntrollers/medusa/medusaconfiguration_controller.go 83.01% <83.01%> (ø)
apis/medusa/v1alpha1/medusaconfiguration_types.go 5.55% <5.55%> (ø)

... and 6 files with indirect coverage changes

@adejanovski adejanovski marked this pull request as ready for review January 19, 2024 14:58
@adejanovski adejanovski requested a review from a team as a code owner January 19, 2024 14:58
)

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.
Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpick] Can we remove those generated comments (here, in the spec below, and in the controller)? It makes it look like there's some unfinished work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good 👍

}

configuration := instance.DeepCopy()
patch := client.MergeFrom(configuration.DeepCopy())
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] why call DeepCopy twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The patch and the original object we're patching need to be in separate addresses for when we invoke .Status().Patch().
I'm not sure why we initially do a DeepCopy() on the object we get from the kube api, but I'm just following the convention we use elsewhere, which could be unnecessary but at least works fine :)

@adejanovski adejanovski force-pushed the medusa-configuration-api branch from c231e2a to 8af65ea Compare January 23, 2024 19:15
@adejanovski adejanovski merged commit 6dc5d42 into main Jan 23, 2024
9 checks passed
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

9.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

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.

Create a new MedusaConfiguration API
2 participants