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

Remove install setup and use from common repo #92

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Remove install setup and use from common repo #92

merged 1 commit into from
Apr 26, 2024

Conversation

Koncpa
Copy link
Contributor

@Koncpa Koncpa commented Mar 7, 2024

Also adjust upstream .fmf plan and check channel
just under condition, if it's deployment provided
via oc client.

@Koncpa Koncpa requested a review from sarroutbi March 7, 2024 13:44
@Koncpa Koncpa self-assigned this Mar 7, 2024
@Koncpa Koncpa force-pushed the pk_minor_fixes branch 2 times, most recently from 97c8619 to 96256fa Compare March 8, 2024 08:51
@Koncpa
Copy link
Contributor Author

Koncpa commented Mar 8, 2024

/packit test

@Koncpa
Copy link
Contributor Author

Koncpa commented Mar 19, 2024

/packit test

sarroutbi
sarroutbi reviewed Mar 19, 2024
View reviewed changes
Setup/creating_test_namespace/runtest.sh Outdated
@@ -59,6 +59,8 @@ rlJournalStart
rlRun "ocpopCheckPodStateAndContinues Running ${TIMEOUT_CONTROLLER_KEEPS_RUNNING} ${OPERATOR_NAMESPACE} ${controller_name}" 0 \
"Checking controller POD continues Running [${TIMEOUT_CONTROLLER_KEEPS_RUNNING} secs.]"
#SECENGSP-5573 Issue
rlRun "ocpopCheckOperatorChannel tang-operator stable"
if [ "${OPERATOR_DEPLOYMENT_CLI}" == "true" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this OPERATOR_DEPLOYMENT_CLI environment variable defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll be defined for downstream plans where is used deployment via CLI.

Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be defined for downstream plans where is used deployment via CLI.

Then, if possible, I would change it to contain something that references DOWNSTREAM, for it to be more clear.

@Koncpa
Copy link
Contributor Author

Koncpa commented Mar 28, 2024

/packit test

@Koncpa Koncpa force-pushed the pk_minor_fixes branch 2 times, most recently from e130ac1 to 9b04b6f Compare March 28, 2024 14:13
@sarroutbi sarroutbi mentioned this pull request Apr 5, 2024
Merged
@Koncpa
Copy link
Contributor Author

Koncpa commented Apr 8, 2024

There is issue with version of GO on used releases is version of GO around 1.21, but when building a image need go >= 1.22.0.

go: downloading sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20240327193027-21368602d84b go: sigs.k8s.io/controller-runtime/tools/setup-envtest@latest: sigs.k8s.io/controller-runtime/tools/[email protected] requires go >= 1.22.0 (running go 1.21.8; GOTOOLCHAIN=local) make: *** [Makefile:247: /var/tmp/tang-operator_sources/bin/setup-envtest] Error 1

What we need to is use rawhide release, where we could get newer version of GO, but there is another issue with panic error of building image which we discussed before. My suggestion is run it just for Fedora-Rawhide and upgrade version for GO 1.22, but we need to handle that panic errors. @sarroutbi WDYT?

@sarroutbi
Copy link
Contributor

There is issue with version of GO on used releases is version of GO around 1.21, but when building a image need go >= 1.22.0.

go: downloading sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20240327193027-21368602d84b go: sigs.k8s.io/controller-runtime/tools/setup-envtest@latest: sigs.k8s.io/controller-runtime/tools/[email protected] requires go >= 1.22.0 (running go 1.21.8; GOTOOLCHAIN=local) make: *** [Makefile:247: /var/tmp/tang-operator_sources/bin/setup-envtest] Error 1

What we need to is use rawhide release, where we could get newer version of GO, but there is another issue with panic error of building image which we discussed before. My suggestion is run it just for Fedora-Rawhide and upgrade version for GO 1.22, but we need to handle that panic errors. @sarroutbi WDYT?

Sorry, I don't understand your point ... it seems that the fix works properly in Rawhide, right? I still need to review the issue with Go 1.22, but I don't believe this is the issue here, as Rawhide seems to work appropriately ...

BTW, why do you need to compile the code? Shouldn't it be enough to download "latest" image from quay.io?

@Koncpa
Copy link
Contributor Author

Koncpa commented Apr 8, 2024 •
edited
Loading

Yeah, but for rawhide we don't install upstream operator.. also when you look more careful into f38 and f39 results, there is error which I printed above, it's just false positive result, it's easy to miss. [1]

[1] https://artifacts.dev.testing-farm.io/d9dfeaed-4a30-43aa-a0ea-048fba4f8eb9/work-upstream-operator-all-testss_qvztox/Plans/upstream-operator-all-tests/execute/data/guest/default-0/Configure_test_system/Setup/install_upstream_operator-2/output.txt

@Koncpa
Copy link
Contributor Author

Koncpa commented Apr 8, 2024

And about compilation, we agreed, that could be useful in case, when we would like to check how upstream changes affect testing..also in future, when we setup CI for upstream code, we could use that upstream task for testing in upstream PR's. It's much easier to compile it, than every change upload it to quay.io..

@sarroutbi
Copy link
Contributor

My suggestion to fix this, at this moment, is to not build the operator (this should not be the tests mission), and use, if possible, the image: quay.io/sec-eng-special/tang-operator-bundle:latest, as this is the latest official image for tang-operator

@sarroutbi
Copy link
Contributor

And about compilation, we agreed, that could be useful in case, when we would like to check how upstream changes affect testing..also in future, when we setup CI for upstream code, we could use that upstream task for testing in upstream PR's. It's much easier to compile it, than every change upload it to quay.io..

Regarding this, IMHO, you have to be aware that non official builds could be causing issues in test execution. tang-operator is a "released early, released often" operator, that is released very frequently, so I would not mess with its compilation in test execution (I guess tests are not normally compiling RPMs, and they just use the RPMs required for the tests, normally the latest ones). But it is a matter of taste ... if you wish to keep compiling it, then you have to wait for the operator to work with latest Golang version, 1.22, which is not planned in the short term

@Koncpa
Copy link
Contributor Author

Koncpa commented Apr 8, 2024

For now we use both option, but I would at least open issue for it in tang-operator upstream to address issue, with panic errors.

@Koncpa
Copy link
Contributor Author

Koncpa commented Apr 8, 2024

I would not set high priority here, but I would at least file that issue. Everybody, who will try to build upstream could met with same issue.

@Koncpa Koncpa force-pushed the pk_minor_fixes branch 2 times, most recently from 763a0a6 to 8e3c99c Compare April 9, 2024 08:10
@sarroutbi
Copy link
Contributor

sarroutbi commented Apr 9, 2024 •
edited
Loading

For now we use both option, but I would at least open issue for it in tang-operator upstream to address issue, with panic errors.

Absolutely. It was opened some time ago: latchset/tang-operator#248

I have it in my TODO list

@Koncpa Koncpa force-pushed the pk_minor_fixes branch 2 times, most recently from 706a9fe to 79cbe23 Compare April 9, 2024 12:42
@Koncpa
Copy link
Contributor Author

Koncpa commented Apr 9, 2024

/packit test

2 similar comments
@Koncpa
Copy link
Contributor Author

Koncpa commented Apr 23, 2024

/packit test

@Koncpa
Copy link
Contributor Author

Koncpa commented Apr 25, 2024

/packit test

@sarroutbi
Copy link
Contributor

For now we use both option, but I would at least open issue for it in tang-operator upstream to address issue, with panic errors.

Absolutely. It was opened some time ago: latchset/tang-operator#248

I have it in my TODO list

@Koncpa : latchset/tang-operator#248 has been resolved. It should work now

sarroutbi
sarroutbi approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@Koncpa
Remove install setup and use from common repo
ed8875c 
Also adjust upstream .fmf plan and check channel
just under condition, if it's deployment provided
via oc client.
@Koncpa Koncpa merged commit d964bf5 into main Apr 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarroutbi sarroutbi sarroutbi approved these changes

Assignees

@Koncpa Koncpa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Koncpa @sarroutbi