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

Skipping integTest for rpm and deb distribution for CCR #4905

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

nandnkum
Copy link
Contributor

@nandnkum nandnkum commented Aug 2, 2024

Skipping integTest for cross-cluster-replication component when running on deb and rpm distribution.

Signed-off-by: nandan <nandnkum@amazon.com>
Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Hi @nandnkum ,

Add some comments.

Once you resolve those I will check if we need test cases update. Thanks.

@@ -172,6 +172,11 @@ pipeline {
echo "Skipping tests for ${component_check} as is not present in the provided build manifest."
componentList -= component_check
}
// Skipping integTest for cross-cluster-replication component when running on deb and rpm distribution
Copy link
Member

Choose a reason for hiding this comment

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

Spacing.

Copy link
Member

Choose a reason for hiding this comment

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

Please also add TODO to this comment and link the corresponding issue.
Eventually we would need to fix this as this change is temporary.

Thanks.

@@ -172,6 +172,11 @@ pipeline {
echo "Skipping tests for ${component_check} as is not present in the provided build manifest."
componentList -= component_check
}
// Skipping integTest for cross-cluster-replication component when running on deb and rpm distribution
if ((distribution.equals('rpm') || distribution.equals('deb')) && component_check == 'cross-cluster-replication') {
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep consistent on using equals vs ==.

…t when running on deb and rpm distribution.

Signed-off-by: nandan <nandnkum@amazon.com>
Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Thanks @nandnkum .

cc: @prudhvigodithi whether we need to merge this next week after 2.16.0 is out. (convert back to draft just in case)

Or we can test earlier on this. Thanks.

@gaiksaya
Copy link
Member

gaiksaya commented Aug 2, 2024

Please wait until 2.16.0 is out. If the pipeline breaks due to syntax or other issues, release process might be stuck.
Thanks!

Also @nandnkum Is there no plan to support CCR on these distributions in the long term?

@peterzhuamazon peterzhuamazon marked this pull request as draft August 2, 2024 18:49
@nisgoel-amazon
Copy link

Also @nandnkum Is there no plan to support CCR on these distributions in the long term?

@gaiksaya for now we don't have any plan to fix these tests on deb/rpm. we need support from release infra team to help over here to create multiple cluster on same ec2.

@gaiksaya
Copy link
Member

gaiksaya commented Aug 2, 2024

Also @nandnkum Is there no plan to support CCR on these distributions in the long term?

@gaiksaya for now we don't have any plan to fix these tests on deb/rpm. we need support from release infra team to help over here to create multiple cluster on same ec2.

I see! Trying to think of a better way to skip for specific use-cases (like this one) instead of adding conditional statements. @peterzhuamazon @prudhvigodithi Do you think we can add something in test-manifest to handle this?
We can create an issue and discuss there too.

@peterzhuamazon
Copy link
Member

peterzhuamazon commented Aug 2, 2024

Also @nandnkum Is there no plan to support CCR on these distributions in the long term?

@gaiksaya for now we don't have any plan to fix these tests on deb/rpm. we need support from release infra team to help over here to create multiple cluster on same ec2.

I see! Trying to think of a better way to skip for specific use-cases (like this one) instead of adding conditional statements. @peterzhuamazon @prudhvigodithi Do you think we can add something in test-manifest to handle this? We can create an issue and discuss there too.

We can, but that would take a bit more time.
For now I am ok to add this condition as we already added a TODO section and we can fix later.
Thanks.

@peterzhuamazon peterzhuamazon marked this pull request as ready for review August 8, 2024 18:13
@peterzhuamazon peterzhuamazon merged commit b6040a3 into opensearch-project:main Aug 8, 2024
10 checks passed
@peterzhuamazon
Copy link
Member

Able to skip, tho not perfect, we should look into directly making the call in python code later.
https://build.ci.opensearch.org/job/integ-test/8532/console

Thanks.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants