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

✨ ADD scheme package for AddToScheme #770

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

acmenezes
Copy link
Contributor

@acmenezes acmenezes commented Apr 19, 2024

Description

This PR adds a scheme package providing a central location for new apis to be added to scheme.

Also replaces the use of a new scheme instance and var by importing the package scheme and using scheme.Scheme.

Solves #322

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@acmenezes acmenezes requested a review from a team as a code owner April 19, 2024 20:10
Copy link

netlify bot commented Apr 19, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit b70e6ed
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/662aabf464a1e10008cb48a7
😎 Deploy Preview https://deploy-preview-770--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@acmenezes
Copy link
Contributor Author

Hi @m1kola , I'm not sure if this is what you expected when you opened the issue #322. I'm just starting getting acquainted with the code base you have here. I didn't catch a few lines here not sure exactly how they are being used.

perdasilva
perdasilva previously approved these changes Apr 25, 2024
Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.21%. Comparing base (265d60d) to head (b70e6ed).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #770      +/-   ##
==========================================
+ Coverage   67.16%   67.21%   +0.04%     
==========================================
  Files          22       23       +1     
  Lines        1465     1467       +2     
==========================================
+ Hits          984      986       +2     
  Misses        415      415              
  Partials       66       66              
Flag Coverage Δ
e2e 45.53% <100.00%> (+0.07%) ⬆️
unit 61.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

That's what I had in mind, thanks for looking into it.

It looks like imports need to be sorted and we should be good to go.

I didn't catch a few lines here not sure exactly how they are being used.

I think it is dead code. We can probably just remove it.

@acmenezes
Copy link
Contributor Author

That's what I had in mind, thanks for looking into it.

It looks like imports need to be sorted and we should be good to go.

I didn't catch a few lines here not sure exactly how they are being used.

I think it is dead code. We can probably just remove it.

Hi @m1kola , I remove those few lines but I saw some linting errors from a tool called gci but I couldn't determine what version you use to lint the project. Could point me to the right github repo? Then I can run it and do a clean up in order to merge the code. Thanks!

@acmenezes
Copy link
Contributor Author

Hi @m1kola , I remove those few lines but I saw some linting errors from a tool called gci but I couldn't determine what version you use to lint the project. Could point me to the right github repo? Then I can run it and do a clean up in order to merge the code. Thanks!

Never mind actually. I figured it out. Bingo! It's fixed now. I believe we're good to go. Thanks.

Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks you!

Never mind actually. I figured it out. Bingo!

Great! Take a look at make help - our Makefile has some useful commands for development. Like make run will spin up a Kind cluster with all the required components installed + will compile operator-controller from local source code and deploy it as well.

If you have any questions about development process - don't hesitate to ask on issues/PR or on Kubernetes slack (channel #olm-dev).

@m1kola m1kola added this pull request to the merge queue Apr 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 26, 2024
@m1kola
Copy link
Member

m1kola commented Apr 26, 2024

Trying to merge again. I think I introduced a flake in #785 and that's why e2e failed in merge queue.

@m1kola m1kola added this pull request to the merge queue Apr 26, 2024
Merged via the queue into operator-framework:main with commit 712cd04 Apr 26, 2024
16 of 17 checks passed
@acmenezes
Copy link
Contributor Author

Looks good. Thanks you!

Never mind actually. I figured it out. Bingo!

Great! Take a look at make help - our Makefile has some useful commands for development. Like make run will spin up a Kind cluster with all the required components installed + will compile operator-controller from local source code and deploy it as well.

If you have any questions about development process - don't hesitate to ask on issues/PR or on Kubernetes slack (channel #olm-dev).

Thanks @m1kola !

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.

None yet

3 participants