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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat: add kompose client PoC #1593

Merged
merged 9 commits into from
Aug 24, 2023
Merged

Feat: add kompose client PoC #1593

merged 9 commits into from
Aug 24, 2023

Conversation

AhmedGrati
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implement a PoC that adds a Kompose client.
Note: For now it's just a PoC. @cdrage and @hangyan your input is really valuable here.

Which issue(s) this PR fixes:

Fixes #464.

Special notes for your reviewer:

@AhmedGrati AhmedGrati added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2023
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 19, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 19, 2023
@AhmedGrati AhmedGrati added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 7, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 7, 2023
@cdrage
Copy link
Member

cdrage commented Jul 10, 2023

/remove-lifecycle rotten

@Mahajanet Mahajanet mentioned this pull request Jul 11, 2023
Open
@cdrage cdrage removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2023
cdrage
cdrage reviewed Jul 17, 2023
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

@AhmedGrati The work you've done is absolutely excellent. I'm super excited to get this merged in one day because we've been missing a library / client for ages and it'd be wonderful to have.

One question though is why there are changes to the tests? Since we're just adding the client, I don't think anything should be changing in script/test/fixtures ?

We may need to rebase I believe.

I did test this out and it worked wonderfully :) I would like to go through this a bit more and create some documentation though, as well as a new section of the site so that people can use the library like so.

@AhmedGrati
Copy link
Contributor Author

@cdrage thanks a lot 🙏, I will go through another iteration in the upcoming days to remove these changes in the test files, rebase and add other missing options in the convert command.

@AhmedGrati AhmedGrati mentioned this pull request Jul 18, 2023
Closed
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 31, 2023
@AhmedGrati AhmedGrati force-pushed the feat-add-kompose-client branch 2 times, most recently from 90c537d to 85ce367 Compare July 31, 2023 14:31
@AhmedGrati
Copy link
Contributor Author

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 31, 2023
@AhmedGrati
Copy link
Contributor Author

@cdrage it should be good now. Can you take a look at it, please?

@AhmedGrati AhmedGrati removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2023
@AhmedGrati AhmedGrati requested a review from cdrage August 1, 2023 15:36
@cdrage
Copy link
Member

cdrage commented Aug 1, 2023

@cdrage it should be good now. Can you take a look at it, please?

Got it! I'll take a review this week. Thank you 🙏

@cdrage
Copy link
Member

cdrage commented Aug 22, 2023

Your code always surprises me! I honestly have no input.. the way that you structured the input parameters and the conversion method makes A LOT of sense and how you return the data as well.

All of this LGTM and thank you for this amazing (and big!) task.

We should open an issue to create documentation on how to actually use this.

You good if I make a release this week so people can use the library?

cdrage
cdrage reviewed Aug 22, 2023
View reviewed changes
client/convert.go
return nil, err
}
objects, err := app.Convert(kobjectConvertOptions)
return objects, err
Copy link
Member

Choose a reason for hiding this comment

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

I like the fact you return the object and then the user can convert that to yaml / json with their own library, as well as the error output. Good job!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AhmedGrati, cdrage

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:

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

@AhmedGrati
Copy link
Contributor Author

@cdrage thanks a lot 😊😊 Yeah We can make a release this week. Should we merge this PR?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2023
@TessaIO
fix: support host port and protocol in functional tests
56a5ea7
@TessaIO
feat: add kompose client with options
8fdfaf2 
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@TessaIO
test: add options unit tests
54dd9e9 
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@TessaIO
feat: add partial convert options
5ae312d 
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@TessaIO
feat: finish convert process
0c2ce0c 
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@TessaIO
test: finish unit tests of the kompose client
c6e9155 
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@TessaIO
remove unecessary changes
e430c48 
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@TessaIO
feat: add generate network policies to client
fc71870 
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2023
@cdrage
Copy link
Member

cdrage commented Aug 23, 2023

Yup! If you can rebase + make sure tests pass (since other PR's got merged in) as soon as it's green, merge it in!

@realgam3
Copy link
Contributor

That's an amazing PR!
I wrote my tool kompose-ex in Python because I didn't had this...
I'm waiting for this 🙏

@TessaIO
update go mod
738e577 
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@AhmedGrati AhmedGrati merged commit ea80734 into kubernetes:main Aug 24, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdrage cdrage cdrage approved these changes

@hangyan hangyan Awaiting requested review from hangyan

@sebgoa sebgoa Awaiting requested review from sebgoa

Assignees
No one assigned
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. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

using kompose as a library
6 participants
@AhmedGrati @k8s-triage-robot @cdrage @k8s-ci-robot @realgam3 @TessaIO