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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃尡 fix and improve samples as add further coverage #3616

Merged
merged 1 commit into from Sep 19, 2023
Merged

馃尡 fix and improve samples as add further coverage #3616

merged 1 commit into from Sep 19, 2023

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Sep 12, 2023 •
edited

Description

  • Fix sample generation. project-v4 should not scaffold the apis with deploy image
  • Add new sample to check project v4 with deploy image plugin and multigroup layout
  • Add CI to run make test and lint for project-v4 samples using deploy image which are the layouts more complex
  • Fix code source by removing unused and duplicate code from generate.sh

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 12, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 12, 2023
@camilamacedo86 camilamacedo86 force-pushed the add-lint-check-sample branch 3 times, most recently from 5504e63 to d1dcacc Compare September 12, 2023 08:52
@camilamacedo86 camilamacedo86 changed the title improve samples and ci checks 馃尡 fix and improve samples as add further coverage Sep 12, 2023
@camilamacedo86 camilamacedo86 marked this pull request as ready for review September 12, 2023 08:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2023
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-27-3

@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-28-0

varshaprasad96
varshaprasad96 reviewed Sep 13, 2023
View reviewed changes
test/testdata/generate.sh

# Call ALL optional plugins within multigroup layout to ensure that they can work within
header_text 'Creating Memcached API with deploy-image plugin ...'
$kb create api --group example.com --version v1alpha1 --kind Memcached --image=memcached:1.4.36-alpine --image-container-command="memcached,-m=64,-o,modern,-v" --image-container-port="11211" --run-as-user="1001" --plugins="deploy-image/v1-alpha" --make=false
Copy link
Member

Choose a reason for hiding this comment

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

@camilamacedo86 why remove this? Even if its v4, we still need to test if we scaffold out 2 APIs. Or am I missing the code for it somewhere out of the diff files?

Copy link
Member Author

Choose a reason for hiding this comment

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

See that this code was duplicated and it was not called only for project-v4-withdeploy-image sample
Therefore, this code was scaffolding the APIs using deploy image for the project-v4 sample when should not.

We should only to do this scaffold for the samples which specific uses the deploy-image.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2023
varshaprasad96
varshaprasad96 approved these changes Sep 18, 2023
View reviewed changes
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, varshaprasad96

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [camilamacedo86,varshaprasad96]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2023
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023
@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2023
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-28-0

@k8s-ci-robot k8s-ci-robot merged commit f474467 into kubernetes-sigs:master Sep 19, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varshaprasad96 varshaprasad96 varshaprasad96 approved these changes

@Kavinjsir Kavinjsir Awaiting requested review from Kavinjsir

@rashmigottipati rashmigottipati Awaiting requested review from rashmigottipati

Assignees

@varshaprasad96 varshaprasad96

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@camilamacedo86 @k8s-ci-robot @varshaprasad96