-
Notifications
You must be signed in to change notification settings - Fork 0
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
COSI 74, COSI 75: Brownfield use case (re-use existing S3 buckets in Kube) #68
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
@@ Coverage Diff @@
## main #68 +/- ##
==========================================
- Coverage 93.46% 93.40% -0.07%
==========================================
Files 9 9
Lines 643 637 -6
==========================================
- Hits 601 595 -6
Misses 36 36
Partials 6 6 |
7b39705
to
3b66088
Compare
e8481a9
to
535757d
Compare
If the bucket already exists, users should add them through brownfield workflow. It is not expected in COSI specs to silently pass if bucket already owned by the IAM user or account used for create bucket request.
3b66088
to
4d96040
Compare
log_and_run kubectl delete -f cosi-examples/bucketclass-delete-on-claim-removal.yaml || { echo "Bucket Class not found." | tee -a "$LOG_FILE"; } | ||
log_and_run kubectl delete -f cosi-examples/greenfield/bucketclass.yaml || { echo "Bucket Class not found." | tee -a "$LOG_FILE"; } | ||
log_and_run kubectl delete -f cosi-examples/greenfield/bucketclaim.yaml || { echo "Bucket Claim not found." | tee -a "$LOG_FILE"; } | ||
log_and_run kubectl delete -f cosi-examples/greenfield/bucketclass-deletion-policy.yaml || { echo "Bucket Class not found." | tee -a "$LOG_FILE"; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this line the name and directory was changed.
This check-in went unchecked in an older PR with the old name of the yaml file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll recap to verify that I'm understanding everything correctly:
- brownfield means that you were already using a bucket for an application before the introduction of cosi. Therefore, you will need to import this existing bucket into cosi.
- you removed the
BucketIsAlreadyOwnedByYou
case since now there is an actual path for brownfield. In other words, we now know thatBucketIsAlreadyOwnedByYou
is not the legit path for the brownfield use case.
log_and_run echo "Protocols mismatch! Expected: $EXPECTED_PROTOCOLS, Found: $ACTUAL_PROTOCOLS" | ||
exit 1 | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could perform some action on the bucket since we have everything we need at this stage, even just a HeadBucket
or ListObjects
, if that's not too complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test in the lastest review commit
100% accurate. |
89e0dfc
to
a3e7337
Compare
Brownfield use case.
Commit # 1: remove silent success if the user already owns the bucket - this is a brownfield case
Commit # 2: Add CRs (custom resources yaml files) for the brownfield use case.
Commit # 3: Added e2e tests for brownfield use case
Commit # 4: Reorganized greenfield CRs in a greenfield folder
Note to reviewers: It is intentional that brownfield CRs are in the same namespace as cosi driver as the sidecar in the cosi driver pod(controlled by CNCF) needs to patch events and that cannot be done cross namespace Reference
Code reference from k8s
Documentation coming soon
Context for branch name: It started with a bug fix, but we ended up understanding how brownfield case works.