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

refactoring testpmd operator to operator-sdk 1.33.0 #57

Merged
merged 3 commits into from
Feb 22, 2024
Merged

refactoring testpmd operator to operator-sdk 1.33.0 #57

merged 3 commits into from
Feb 22, 2024

Conversation

capolrik
Copy link
Collaborator

@capolrik capolrik commented Feb 15, 2024 •
edited
Loading

After Testpmd-lb-operator and cnf-mac-app-operator, here it comes the refactoring of testpmd-operator

@capolrik capolrik requested a review from ramperher February 15, 2024 13:49
@dcibot
Copy link

dcibot commented Feb 15, 2024

from change #57:

  • ERROR no DCI job found

@dcibot
Copy link

dcibot commented Feb 15, 2024

from change #57:

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from bd0ae3a to d791303 Compare February 16, 2024 10:03
ramperher
ramperher requested changes Feb 16, 2024
View reviewed changes
Copy link
Collaborator

@ramperher ramperher left a comment

Choose a reason for hiding this comment

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

Some comments, comparing with #53:

  • Include in the README the operator-sdk commands executed to create the project structure
  • roles/testpmd/vars/main.yml file is missed (it's just an empty file but just to have the same than in testpmd-lb-operator)

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from d791303 to 42c50b2 Compare February 16, 2024 14:33
ramperher
ramperher requested changes Feb 16, 2024
View reviewed changes
Copy link
Collaborator

@ramperher ramperher left a comment

Choose a reason for hiding this comment

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

second round of reviews, also, I saw this is still missed:

  • roles/testpmd/vars/main.yml file is missed (it's just an empty file but just to have the same than in testpmd-lb-operator)

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch 2 times, most recently from e2fe2dc to 96c78e1 Compare February 19, 2024 15:59
@ramperher
Copy link
Collaborator

Change looks good, I'll accept when having a green job with a full example-cnf deployment and testing

@dcibot
Copy link

dcibot commented Feb 19, 2024

from change #57:

@dcibot
Copy link

dcibot commented Feb 19, 2024

from change #57:

@ramperher
Copy link
Collaborator

it's weird but it's complaining about this script: https://github.com/dci-labs/example-cnf-config/blob/master/testpmd/hooks/pre-run.yml#L29-L33 , which mainly retrieves the operators that are modified to retrieve the correct SHA. It's like it's not able to see the change in testpmd-operator?

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from 96c78e1 to 5b4d6d6 Compare February 19, 2024 17:03
@dcibot
Copy link

dcibot commented Feb 19, 2024

from change #57:

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from 5b4d6d6 to 1e05072 Compare February 20, 2024 08:45
@dcibot
Copy link

dcibot commented Feb 20, 2024

from change #57:

@dcibot
Copy link

dcibot commented Feb 20, 2024

from change #57:

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from 927a36a to 6e232cb Compare February 20, 2024 10:46
@dcibot
Copy link

dcibot commented Feb 20, 2024

from change #57:

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch 2 times, most recently from 26e8bef to f95ea32 Compare February 20, 2024 13:39
@dcibot
Copy link

dcibot commented Feb 20, 2024

from change #57:

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from f95ea32 to d202e8b Compare February 20, 2024 13:55
@dcibot
Copy link

dcibot commented Feb 20, 2024

from change #57:

@dcibot
Copy link

dcibot commented Feb 20, 2024

from change #57:

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from d202e8b to cbb3357 Compare February 20, 2024 15:39
@dcibot
Copy link

dcibot commented Feb 20, 2024

from change #57:

@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from cbb3357 to 3ec2abe Compare February 20, 2024 16:27
@capolrik capolrik force-pushed the testpmd-operator-sdk-1.33 branch from 3ec2abe to 7362599 Compare February 21, 2024 10:34
@dcibot
Copy link

dcibot commented Feb 21, 2024

from change #57:

@capolrik capolrik requested a review from ramperher February 22, 2024 08:22
@capolrik
Copy link
Collaborator Author

I fix the dependency issue: the pip install cmd was not run under the correct USER in the dockerfile.

ramperher
ramperher approved these changes Feb 22, 2024
View reviewed changes
Copy link
Collaborator

@ramperher ramperher left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Charles for all the great work here!

Let's wait for PR54 to build the example-cnf component before merging this one, to avoid conflicts: https://github.com/openshift-kni/example-cnf/actions/runs/8001713950

@ramperher ramperher merged commit a51eb21 into main Feb 22, 2024
1 check passed
@tonyskapunk tonyskapunk deleted the testpmd-operator-sdk-1.33 branch November 20, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramperher ramperher ramperher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@capolrik @dcibot @ramperher