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

Add the fingerprint updater and .binproto for Apache Solr #436

Merged
merged 17 commits into from
Oct 22, 2024

Conversation

W0ngL1
Copy link
Contributor

@W0ngL1 W0ngL1 commented Mar 23, 2024

Hi @tooryx,

This PR is about #395.

The solr team started to build docker images from 8 years ago, the first version which has a official docker is 5.3.1, but versions from 5.3.1 to 5.5.0 cannot be launched using official examples due to missing files. So the actual fingerprint only covers versions 5.5.1(8 years ago) to 9.5.0(2024.01).

And the version 8.1.0 has only one alias for 8.1 in hub.docker.com, so there is an IF check for it in update.sh.

#408 was deleted by mistake, so I reopen one.

@tooryx tooryx added Contributor main The main issue a contributor is working on (top of the contribution queue). fingerprints labels Aug 6, 2024
@lokiuox lokiuox self-requested a review September 4, 2024 13:47
Copy link
Collaborator

@lokiuox lokiuox left a comment

Choose a reason for hiding this comment

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

Hey @W0ngL1, thanks for your contribution!

I'm reviewing your plugin and I confirm it's working. I've added a few comments on little things that can be improved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The file is missing the executable bit, please add it

releases/lucene-solr/5.5.3
releases/lucene-solr/5.5.4
releases/lucene-solr/5.5.5
releases/lucene-solr/6.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having issues with version 6.0.0, docker complains that the image file is in the incorrect format. Do you have the same issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't look like it, I checked the temporary fingerprint file from the last run.

...
-rw-r--r-- 1 root root 259787  3月  8  2024 fingerprint.6.5.1.json
-rw-r--r-- 1 root root 275376  3月  8  2024 fingerprint.6.6.0.json
-rw-r--r-- 1 root root 290605  3月  8  2024 fingerprint.6.6.1.json
...

@W0ngL1
Copy link
Contributor Author

W0ngL1 commented Sep 18, 2024

Hi @lokiuox, feel free to review it.

Copy link
Collaborator

@lokiuox lokiuox left a comment

Choose a reason for hiding this comment

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

Hey @W0ngL1, the fingerprinter seems fine, but there are still a few things to fix.

Besides the comments in the review, please also address these things:

  • Set the executable bit on the update.sh file
  • I see that there are a few old versions still missing for which there are images available, i.e. v5.3.x and v5.4.x. If possible please add them too.

@W0ngL1
Copy link
Contributor Author

W0ngL1 commented Sep 24, 2024

Hi @lokiuox,

I've updated version.txt. Versions 5.3.0/5.3.1/5.3.2/5.4.0/5.4.1 can be found in docker hub, but versions 5.3.1/5.4.0 are no longer allowed to be pulled.
I'll update .binproto file soon.

I don't understand what Set the executable bit on the update.sh file means, like chmod +x update.sh?

@lokiuox
Copy link
Collaborator

lokiuox commented Sep 24, 2024

You can use this command to mark the file as executable: git update-index --chmod=+x update.sh.
If it doesn't work, try removing it and adding it again:

git update-index --chmod=-x update.sh
git update-index --chmod=+x update.sh

@W0ngL1
Copy link
Contributor Author

W0ngL1 commented Sep 24, 2024

You can use this command to mark the file as executable: git update-index --chmod=+x update.sh. If it doesn't work, try removing it and adding it again:

git update-index --chmod=-x update.sh
git update-index --chmod=+x update.sh

Copy that.

@W0ngL1
Copy link
Contributor Author

W0ngL1 commented Sep 24, 2024

Hi @lokiuox, I delete files and re-clone it, and update.sh has execution permission.

@lokiuox
Copy link
Collaborator

lokiuox commented Sep 24, 2024

LGTM - Approved
@maoning you can merge it.

Reviewer: Savio, Doyensec
Plugin: Apache Solr Fingerprinter
Feedback: The version.txt was missing a few versions, and the script needed a few small improvements, but it worked first try and the overall quality is good.
Drawbacks: None

@tooryx tooryx added the lgtm label Sep 30, 2024
@tooryx
Copy link
Member

tooryx commented Oct 22, 2024

Hi @W0ngL1,

This PR is being reviewed internally and should be merged in a few days.

~tooryx

@W0ngL1
Copy link
Contributor Author

W0ngL1 commented Oct 22, 2024

Thank you @tooryx.

@copybara-service copybara-service bot merged commit 91cbf07 into google:master Oct 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contributor main The main issue a contributor is working on (top of the contribution queue). fingerprints lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants