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

[feat] add a v1alpha2 version of LinodeObjectStorageBucket #427

Merged
merged 11 commits into from
Jul 31, 2024

Conversation

unnatiagg
Copy link
Contributor

What this PR does / why we need it:
This PR adds LinodeObjectStorageBucket to the v1alpha2 version
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
This is the Step 2: for the two fold update mentioned in PR
Special notes for your reviewer:

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 57.29167% with 41 lines in your changes missing coverage. Please review.

Project coverage is 64.89%. Comparing base (3119045) to head (472980b).
Report is 2 commits behind head on main.

Files Patch % Lines
api/v1alpha2/linodeobjectstoragebucket_webhook.go 51.61% 15 Missing ⚠️
...i/v1alpha1/linodeobjectstoragebucket_conversion.go 42.10% 6 Missing and 5 partials ⚠️
api/v1alpha2/linodeobjectstoragebucket_types.go 33.33% 4 Missing ⚠️
controller/linodeobjectstoragebucket_controller.go 33.33% 3 Missing and 1 partial ⚠️
api/v1alpha1/conversion.go 83.33% 1 Missing and 2 partials ⚠️
cmd/main.go 0.00% 3 Missing ⚠️
...i/v1alpha2/linodeobjectstoragebucket_conversion.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #427      +/-   ##
==========================================
+ Coverage   64.65%   64.89%   +0.23%     
==========================================
  Files          68       72       +4     
  Lines        3517     3606      +89     
==========================================
+ Hits         2274     2340      +66     
- Misses       1061     1081      +20     
- Partials      182      185       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dst.ObjectMeta = src.ObjectMeta

// Spec
dst.Spec.Cluster = src.Spec.Region + "-1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it guaranteed that cluster will be {region}-1?

Of course, I can't imagine a world where anyone would be trying to convert to v1alpha1. Is the reverse conversion from v1alpha2 to v1alpha1 actually necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it is necessarily true that it will always be {region}-1 sounds like this might make downgrading pretty challenging here though

Copy link
Contributor

Choose a reason for hiding this comment

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

^ while it has been true so far, the api now responds with a hostname for a bucket - maybe we can infer the clusterID from it - not always guaranteed to work, but better than -1

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. For successfully provisioned v1alpha1 buckets, the hostname is stored as status.hostname. If no status.hostname has been set (i.e. status.ready is false), then it's probably safe to default to {region}-1 since it'd be a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going forward with {region}-1, while converting from v1alpha2 to v1alpha1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

will v1alpha1 work without that conversion?

@@ -179,6 +179,175 @@ spec:
type: object
type: object
served: true
storage: false
Copy link
Contributor

Choose a reason for hiding this comment

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

you'd need to update the reference in config/crd/patches/capicontract_in_linodeobjectstoragebuckets.yaml to v1alpha2 as well

Copy link
Contributor Author

@unnatiagg unnatiagg Jul 26, 2024

Choose a reason for hiding this comment

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

Hey @tchinmai7 , I have actually added that change in my local version. Will be pushing it out in a bit.

@unnatiagg unnatiagg merged commit bd77f69 into main Jul 31, 2024
11 of 12 checks passed
@AshleyDumaine AshleyDumaine deleted the linodeOBJBucket_v1alpha2 branch August 9, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants