add druid-operator to apache druid#18435
Conversation
capistrant
left a comment
There was a problem hiding this comment.
Thank you for getting this started. Have left high level comments so far regarding things like Apache headers, files we may want to remove as part of this, etc.
There was a problem hiding this comment.
should this be deleted with perhaps a new section added to the base pr template that gives a checklist for PRs that touch druid-operator?
There was a problem hiding this comment.
can we delete? Druid repo should have its own stale PR handler if I remember correctly
There was a problem hiding this comment.
does this need to be moved into base druid github workflows area?
There was a problem hiding this comment.
I assume this needs to be deleted and druid-operator now falls under the core druid LICENSE file, but I'm not up to snuff on the license specifics with project donation to an apache project
There was a problem hiding this comment.
Also a candidate for removal I think
| @@ -0,0 +1,102 @@ | |||
| /* | |||
There was a problem hiding this comment.
I'll only comment once per file ext type :) -- all .go source files need the proper apache copy header
Pulled from our java source files:
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/There was a problem hiding this comment.
I believe all the project YAML files will need the apache copy header.
Example pulled from other druid Yaml file:
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#-------------------------------------------------------------------------
| @@ -0,0 +1,7 @@ | |||
| FROM alpine:3.17.2 | |||
There was a problem hiding this comment.
same license requirements for the Dockerfiles in the repo
pulled from existing Dockerfile in repo
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#-------------------------------------------------------------------------
There was a problem hiding this comment.
same License stuff applies for .sh files
From Druid repo existing script. comment came after shebang line + linebreak
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#-------------------------------------------------------------------------
There was a problem hiding this comment.
I'm unfamiliar with .tpl but I do figure we will need apache copy headers in these too 🤔
|
Thanks for review @capistrant ill address the comments. |
|
Clear and Approved for Apache Donation from my side. |
|
@capistrant - i have addressed all your comments. Regarding .tpl i myself am unsure. This was actually generated as part of repo initialisation. |
|
@AdheipSingh thanks for the updates! I guess there are more files still getting flagged for improper license header in static checks. Once we get this green, it will be easier to see what other things may need to be fixed up. ^ From java 17 static checks listing files missing licenses header. I guess even markdowns need the proper licensing header. You can find examples for each file type in the existing repository. For |
|
@capistrant go.mod and go.sum can't have headers. Rest files i have added. |
I see, then I guess we will have to add Line 2169 in 90be682 |
I do see other projects have license headers in this file: For |
|
@capistrant @yurmix |
capistrant
left a comment
There was a problem hiding this comment.
awesome to see green CI! I have a few more comments
There was a problem hiding this comment.
This template should not be removed
There was a problem hiding this comment.
I think this needs to be deleted. Our other modules do not have module level LICENSE files. This module will fall under the overall project LICENSE after donation
There was a problem hiding this comment.
nit: it could make sense to sync this up with the parent gitignore and remove duplicates. But probably not the biggest deal
|
@capistrant all files removed as you suggested. |
@AdheipSingh I'm still seeing |
|
@capistrant I have added the PR template back. Apologies for confusion. |
|
FYI, I just raised an IP Clearance vote with the Incubator: https://lists.apache.org/thread/s0nrv9ny396nkll396f30c57o5nvdkd4 |
|
IP Clearance has passed. @capistrant do you think this is ready to merge from a code point of view? We can fix up most things after merging. The main thing that should be done pre-merging is ensure CI still passes and that license headers are correct. To me this appears done. |
|
It doesn't need to block this PR, but the next thing that should be discussed is how we handle releases. I posted on the dev thread about it here: https://lists.apache.org/thread/1r3fmpmdjvmptq9ckn36xq6zdd9hgvmd @AdheipSingh @yurmix @razinbouzar — please chime in on the thread with your thoughts. As you all have been most involved in the discussions recently, I am assuming one of you will have an opinion on releases 😄 |
Thanks @gianm as always for your support. I will chime in the thread. |
|
@AdheipSingh could you merge master into this once and then pending green CI I will approve and merge. Thank you for all the work getting this ready to merge. We are almost there. |
|
@AdheipSingh checking in here to see again if you could do a quick merge of master into your branch to run on the latest CI workflows as one last check before we merge and move ahead with integrating druid-operator into druid! |
|
@capistrant - i have merged master into my branch |
|
@capistrant or @gianm can either of you merge this PR? Two issues have been created to track releases: |
|
It would make some sense to have a separate repo with its own release cycle. @capistrant and @AdheipSingh, what else do we need to get this going? |
Essentially reverts apache#18435 since the repo has now been moved to https://github.com/apache/druid-operator as part of the ASF donation.
Fixes #XXXX.
Description
Based on discussion with Druid PMC on threads, i have raised a PR to move druid-operator to druid.