-
Notifications
You must be signed in to change notification settings - Fork 69
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
Ibmcloud e2e #636
Ibmcloud e2e #636
Conversation
Hi @Mathieu-Ferraton. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
eaa6a91
to
cb0e6a8
Compare
improve test filtering and add labels to aws specific test cases
cb0e6a8
to
3129668
Compare
0e9dc22
to
6e5eaea
Compare
/retest |
1 similar comment
/retest |
@@ -155,7 +155,7 @@ items: | |||
spec: | |||
containers: | |||
- name: todolist | |||
image: quay.io/rhn_engineering_whayutin/todolist-mariadb-go:latest | |||
image: quay.io/mferrato/todolist-mariadb-go:v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happening here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wesley's image is amd64 only. I made a PR to the mig-demo-app repo konveyor/mig-demo-apps#74 to add the code enabling multiarch builds for the mariadb-based app. We discused that with @weshayutin as a temporary solution until we have a place to store the multiarch manifest. But I'd rather store it somewhere else, if you know of a better place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.. I've just opened an issue on OADP to better describe what needs to be done here and why. #666
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So konveyor quay org would be one
@@ -381,7 +381,7 @@ var _ = Describe("Configuration testing for DPA Custom Resource", func() { | |||
} | |||
|
|||
awsTests := []TableEntry{ | |||
Entry("AWS Without Region No S3ForcePathStyle with BackupImages false should succeed", InstallCase{ | |||
Entry("AWS Without Region No S3ForcePathStyle with BackupImages false should succeed", Label("aws"), InstallCase{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather see this label added in a for loop instead since that's the whole point of a separate awsTests
declaration. But I am aware this is not possible right now.
onsi/ginkgo#974 might allow this in the future.
Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
/retest |
@Mathieu-Ferraton: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@hhpatel14 @deepakraj1997 please review when you have a moment :) |
TEST_FILTER := $(shell echo '! aws && ! gcp && ! azure' | sed -r "s/[&]* [!] $(CLUSTER_TYPE)|[!] $(CLUSTER_TYPE) [&]*//") | ||
TEST_FILTER := ($(shell echo '! aws && ! gcp && ! azure && ! ibmcloud' | \ | ||
sed -r "s/[&]* [!] $(CLUSTER_TYPE)|[!] $(CLUSTER_TYPE) [&]*//")) || $(CLUSTER_TYPE) | ||
#TEST_FILTER := $(shell echo '! aws && ! gcp && ! azure' | sed -r "s/[&]* [!] $(CLUSTER_TYPE)|[!] $(CLUSTER_TYPE) [&]*//") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit.. probably can remove this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove the commented line, but i think we can keep the test_filter as we can use it to run cloud specific tests in future.
"config": { | ||
"profile": "$BSL_AWS_PROFILE", | ||
"region": "$BSL_REGION", | ||
"s3ForcePathStyle": "true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mathieu-Ferraton @kaovilai Wont this set s3ForcePathStyle
for all the SPECs that are running ? I think we explicitly set these in some tests and remove in some tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepakraj1997 You're right, but I think these are aws specific tests, which we want to skip for ibmcloud clusters (this can be discussed further though). If we use IBM COS bucket as BSL with ibmcloud/powerVS clusters, then we need to keep this to true
, or fork an ibmcloud plugin out of the aws one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepakraj1997 This is fine. It would apply to all specs within the ibmcloud which have to use aws plugin via the s3ForcePathStyle to work.
No description provided.