Skip to content

Conversation

@martint
Copy link
Member

@martint martint commented Dec 5, 2024

These were missed when renaming the plugin module

(x) Release notes are required, with the following suggested text:

## Security

* TBD. ({issue}`24392`)

@cla-bot cla-bot bot added the cla-signed label Dec 5, 2024
@mosabua
Copy link
Member

mosabua commented Dec 5, 2024

Need to update docs as well as some config files used in the tests.

These were missed when renaming the plugin module
@mosabua
Copy link
Member

mosabua commented Dec 5, 2024

Also to clarify for anybody else looking or reviewing .. we do not use the "apache" prefix for any other connector or module or user facing properties and we therefore want to rename this and stay consistent. It also makes it shorter.

Given that the plugin just shipped in 466 the impact on users as a breaking change will be minimal and it is better to do the rename now than later.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks good to me. Heads up @lozbrown @mneethiraj @kokosing and @ksobolew

@martint martint merged commit 1b396e5 into trinodb:master Dec 6, 2024
18 checks passed
@martint martint deleted the ranger-rename branch December 6, 2024 01:54
@github-actions github-actions bot added this to the 467 milestone Dec 6, 2024
@ebyhr
Copy link
Member

ebyhr commented Dec 6, 2024

Could you confirm CI failure on master? https://github.com/trinodb/trino/actions/runs/12191471556/job/34010672844

presto-master       | java.lang.IllegalStateException: Access control 'apache-ranger' is not registered: /var/trino/etc/access-control.properties
presto-master       | 	at com.google.common.base.Preconditions.checkState(Preconditions.java:836)
presto-master       | 	at io.trino.security.AccessControlManager.createSystemAccessControl(AccessControlManager.java:217)
presto-master       | 	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:215)
presto-master       | 	at java.base/java.util.Collections$2.tryAdvance(Collections.java:5075)
presto-master       | 	at java.base/java.util.Collections$2.forEachRemaining(Collections.java:5083)
presto-master       | 	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:570)
presto-master       | 	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:560)
presto-master       | 	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
presto-master       | 	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:265)
presto-master       | 	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:727)
presto-master       | 	at io.trino.security.AccessControlManager.loadSystemAccessControl(AccessControlManager.java:176)
presto-master       | 	at io.trino.server.Server.doStart(Server.java:174)
presto-master       | 	at io.trino.server.Server.lambda$start$0(Server.java:94)
presto-master       | 	at io.trino.$gen.Trino_466_89_g1b396e5____20241206_023127_1.run(Unknown Source)
presto-master       | 	at io.trino.server.Server.start(Server.java:94)
presto-master       | 	at io.trino.server.TrinoServer.main(TrinoServer.java:37)

@mosabua
Copy link
Member

mosabua commented Dec 6, 2024

Not sure how the PR build passed .. but anyway . this should fix it. #24394

@lozbrown
Copy link
Contributor

lozbrown commented Dec 6, 2024

Looks good to me. Heads up @lozbrown @mneethiraj @kokosing and @ksobolew

Cheers for the heads up, have been working on getting this stuff working

@mneethiraj
Copy link
Contributor

@martint, @mosabua, @lozbrown - the plugin was initially named as ranger; it was renamed to apache-ranger based on a review suggestion, to avoid potential conflict with other existing plugin implementations (for example, in Apache Ranger git repo; and other vendors as well).

I don't have preference for one name over other.

@nineinchnick
Copy link
Member

These changes are not mentioned in the release notes, are property renames not considered breaking changes?

@mosabua
Copy link
Member

mosabua commented Dec 9, 2024

Yeah .. we left them out since the docs were updated, the feature was only out for one release and a few days, and we notified the contributors and everyone who worked on it. I am happy to add a note in for completeness anyway if desired.. wdyt @ebyhr @martint @kokosing ?

@lozbrown
Copy link
Contributor

I think a note saying which properties were changed and AB's what the replacements were would be helpful

We can't presume we know everyone who was trying out 466

@churromorales
Copy link
Contributor

yeah this should always be mentioned, it broke for us..we were on 466 and upgraded to 468 and this wasn't mentioned in the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

7 participants