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

Read DOCKER_VERSION from version file instead of env variable #2356

Merged
merged 2 commits into from Sep 16, 2022

Conversation

yubiuser
Copy link
Member

What does this PR aim to accomplish?:

With pi-hole/pi-hole#4913 we store the docker tag (if available) as DOCKER_VERSION in the versions file. This PR makes PHP reading docker_current from that file instead of using the passed env variable. This makes this assignment work in the same way we assign all the other version variables.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser requested a review from a team September 16, 2022 13:58
@yubiuser yubiuser added the PR: Approval Required Open Pull Request, needs approval label Sep 16, 2022
Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser requested a review from a team September 16, 2022 14:05
@yubiuser yubiuser merged commit 45d281c into devel Sep 16, 2022
@yubiuser yubiuser deleted the DOCKER_VERSION_from_file branch September 16, 2022 18:22
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-18-1-web-v5-15-1-and-core-v5-12-2-released/58022/1

@kroese
Copy link

kroese commented Sep 19, 2022

For a long time I had the issue that when updating the pi-hole image using Portainer, it doesn't change any of the existing environment variabeles, causing the DOCKER_VERSION displayed to be incorrect. It always show the tag of the first version that was installed, not the current version.

I really hoped this pull request would fix this problem, but it did not. So does the file just copies the contents of the environment variabele? Isn't possible to fill the file in another way, so that it ignores the environment variabele? Or can I simply delete the variabele to fix this?

@rdwebdesign
Copy link
Member

For a long time I had the issue that when updating the pi-hole image using Portainer, it doesn't change any of the existing environment variabeles, causing the DOCKER_VERSION displayed to be incorrect. It always show the tag of the first version that was installed, not the current version.

I really hoped this pull request would fix this problem, but it did not. So does the file just copies the contents of the environment variabele? Isn't possible to fill the file in another way, so that it ignores the environment variabele? Or can I simply delete the variabele to fix this?

The previous container should be destroyed before using a new image.
This is the recommended way. If you are using volumes, all preferences would be preserved and your compose file can add the options as well.

That said, we are already changing the code (to ignore the var) to avoid confusion when this variable is not removed.

@kroese
Copy link

kroese commented Sep 19, 2022

The previous container should be destroyed before using a new image. This is the recommended way.

Yes, but this is unfortunaly not how Portainer works when it pulls a new image. Also it would mean that I loose all container modifications (like the network, custom IP address, etc). The only alternative would to create a compose file with these modifications, instead of using the Portainer GUI to set them.

That said, we are already changing the code (to ignore the var) to avoid confusion when this variable is not removed.

I just removed the variabele, and v5.12.2 is showing the correct docker tag now I removed it. So my issue is solved, thanks!

@rdwebdesign
Copy link
Member

Yes, but this is unfortunaly not how Portainer works when it pulls a new image.

Not exaclty.
Portainer works correctly if you know where to look for (It's not hidden, but it's easy to miss).

If you want to update the image and keep your config, you can use the Duplicate/Edit button,
BUT you need to remove all old variables to avoid loading stale data (you can keep just the variables you know you will need):

Pihole_Portainer_ENV_VAR

Or you can use Portainer Stacks and start your container using a compose file.

@kroese
Copy link

kroese commented Sep 20, 2022

That is exactly how I always did it: by using Duplicate/Edit. But as you were talking about destroying the container first, I thought you ment something else. Anyhow, it works now.

@rdwebdesign
Copy link
Member

You should destroy it.
Using a compose file to start the container will allow you to destroy it and start a new one with exactly the same config every time.

If you are unable to do it, you can use the method shown above.

That is exactly how I always did it: by using Duplicate/Edit. ...

But did you remove all variables like I did on the video?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Approval Required Open Pull Request, needs approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants