Skip to content

Disable handling of OVERRIDES_DIR / /overrides in product tests#12153

Merged
findepi merged 1 commit intotrinodb:masterfrom
findepi:findepi/disable-handling-of-overrides-in-product-tests-dcf992
Aug 25, 2022
Merged

Disable handling of OVERRIDES_DIR / /overrides in product tests#12153
findepi merged 1 commit intotrinodb:masterfrom
findepi:findepi/disable-handling-of-overrides-in-product-tests-dcf992

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Apr 27, 2022 •
edited
Loading

This seems unused in product tests.

Moreover, apply-all-site-xml-overrides /overrides is done as part of
Docker images build process (see docker-images repo), so it seems we
would be applying same overrides twice.

@findepi findepi added test maintenance Project maintenance task tests:hive tests:all-product Run all product tests no-release-notes This pull request does not require release notes entry labels Apr 27, 2022
@cla-bot cla-bot bot added the cla-signed label Apr 27, 2022
@findepi findepi force-pushed the findepi/disable-handling-of-overrides-in-product-tests-dcf992 branch from bea5a1b to 23ed484 Compare April 27, 2022 08:33
@findepi findepi changed the title Disable handling of /overrides in product tests Disable handling of OVERRIDES_DIR / /overrides in product tests Apr 27, 2022
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Apr 27, 2022

The Ci should fail, as docker images contain /overrides and we currently cannot distinguish between overrides populated (and already applied) during image creation and ones provided by the PT env setup (supposedly never being the case).

cc @wendigo @losipiuk @hashhar @kokosing

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Apr 27, 2022

overrides populated (and already applied) during image creation

Edit: these are removed during image creation already, so we should be fine.

@findepi findepi marked this pull request as ready for review April 27, 2022 08:36
@findepi findepi force-pushed the findepi/disable-handling-of-overrides-in-product-tests-dcf992 branch 2 times, most recently from 260e945 to b9dcd20 Compare April 27, 2022 12:19
@findepi findepi force-pushed the findepi/disable-handling-of-overrides-in-product-tests-dcf992 branch from b978ff4 to dbf8b39 Compare June 9, 2022 08:58
wendigo
wendigo approved these changes Jun 9, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up @findepi

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 9, 2022

There still is /overrides in ghcr.io/trinodb/testing/cdh5.15-hive-kerberized-kms:59.
Looking for someone to help here.
cc @wendigo

@findepi findepi force-pushed the findepi/disable-handling-of-overrides-in-product-tests-dcf992 branch from dbf8b39 to c9f1d73 Compare June 9, 2022 13:34
@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Jun 9, 2022

@findepi were those overrides applied at build time?

@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Jun 9, 2022

@findepi trinodb/docker-images#127

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 10, 2022

were those overrides applied at build time?

they should be.

there was a bunch of images where build-time created /overrides and applied them, and didn't remove these override files.
i think i fixed some, but clearly not all. thanks for taking the lead here

@findepi findepi marked this pull request as draft June 10, 2022 08:43
@hashhar hashhar mentioned this pull request Jun 15, 2022
Merged
@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Jul 1, 2022

This should work after upgrading to docker-images v63. @ebyhr is taking care of it.

@findepi
Disable handling of OVERRIDES_DIR / /overrides in product tests
5b1a9c0 
This seems unused in product tests.

Moreover, `apply-all-site-xml-overrides /overrides` is done as part of
Docker images build process (see `docker-images` repo), so it seems we
would be applying same overrides twice.
@findepi findepi force-pushed the findepi/disable-handling-of-overrides-in-product-tests-dcf992 branch from c9f1d73 to 5b1a9c0 Compare August 24, 2022 09:55
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Aug 24, 2022

rebased

@findepi findepi marked this pull request as ready for review August 24, 2022 09:55
wendigo
wendigo approved these changes Aug 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

LGTM

kokosing
kokosing reviewed Aug 24, 2022
View reviewed changes
...ncher/src/main/resources/docker/presto-product-tests/common/hadoop/apply-config-overrides.sh
@@ -1,14 +1,15 @@
#!/usr/bin/env bash
Copy link
Copy Markdown
Member

@kokosing kokosing Aug 24, 2022

Choose a reason for hiding this comment

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

Why not to drop it all together?

Copy link
Copy Markdown
Contributor

@wendigo wendigo Aug 24, 2022

Choose a reason for hiding this comment

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

I guess to be sure that we do not have overrides in one of the base images

Copy link
Copy Markdown
Member Author

@findepi findepi Aug 24, 2022

Choose a reason for hiding this comment

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

To be sure no-one tries to use mechanism that we're decomissioning.
Otherwise it could be possible that test run with a different configuration than expected.

Copy link
Copy Markdown
Member Author

@findepi findepi Aug 24, 2022

Choose a reason for hiding this comment

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

Removal should be a follow-up sometime later.

@findepi findepi merged commit 138eaaa into trinodb:master Aug 25, 2022
@findepi findepi deleted the findepi/disable-handling-of-overrides-in-product-tests-dcf992 branch August 25, 2022 07:40
@github-actions github-actions bot added this to the 394 milestone Aug 25, 2022
@colebow colebow mentioned this pull request Aug 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wendigo wendigo wendigo approved these changes

@kokosing kokosing kokosing left review comments

@losipiuk losipiuk losipiuk approved these changes

@hashhar hashhar hashhar approved these changes

Assignees

No one assigned

Labels

cla-signed maintenance Project maintenance task no-release-notes This pull request does not require release notes entry test tests:all-product Run all product tests

Milestone

394

Development

Successfully merging this pull request may close these issues.

5 participants

@findepi @wendigo @losipiuk @kokosing @hashhar