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

fix: some minor bugs and make Dockerfile more productive. #831

Merged
merged 4 commits into from
Jun 29, 2022
Merged

fix: some minor bugs and make Dockerfile more productive. #831

merged 4 commits into from
Jun 29, 2022

Conversation

atompi
Copy link
Contributor

@atompi atompi commented Jun 27, 2022

These are some very minor changes, so no demos are provided.

Make Dockerfile more productive.

  • Build all-in-one image:
docker build --target ALLINONE -t casbin/casdoor-all-in-one:latest .
  • Build standard image:
docker build --target STANDARD -t casbin/casdoor:latest .

Solve the problem that the PermissionList page jumped abnormally when clicking the permission name in the list.

  • Insert the owner into the url when jumping to the permissions details page.

Fix incorrect SignupApplication field value in initBuiltInUser.

  • Wrong SignupApplication value causes exceptions in multiple functions for the first admin user, such as uploading avatars, user detail pages, etc.

@casbin-bot
Copy link
Contributor

@Steve0x2a @seriouszyx @Abingcbc @ComradeProgrammer please review

@hsluoyz
Copy link
Member

hsluoyz commented Jun 27, 2022

@ComradeProgrammer plz review

Copy link
Contributor

@ComradeProgrammer ComradeProgrammer left a comment

Choose a reason for hiding this comment

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

Sorry, I cannot see why these changes are applied to dockerfile.

  1. What kind of benefits can we get by spiliting ALLINONE into ALLINONE_DB or ALLINONE because few people build them together
  2. There are a lot of things omitted which exist in old version, and I doubt whether this is proper , for example, RUN apk add ca-certificates && update-ca-certificates

@atompi
Copy link
Contributor Author

atompi commented Jun 28, 2022

Sorry, I cannot see why these changes are applied to dockerfile.

  1. What kind of benefits can we get by spiliting ALLINONE into ALLINONE_DB or ALLINONE because few people build them together
  2. There are a lot of things omitted which exist in old version, and I doubt whether this is proper , for example, RUN apk add ca-certificates && update-ca-certificates

This PR has more changes to the Dockerfile and they are all meaningful changes.

  1. The all-in-one image contains a mariadb-server, and installing the mariadb-server via apt has no dependencies on other build steps, and stripping it out of ALLINONE makes the Dockerfile look hierarchical. Also, placing STANDARD before ALLINONE eliminates the need to build ALLINONE images when building STANDARD images, which greatly optimizes the experience when building STANDARD images.
  2. There are many unnecessary shell commands in the old Dockerfile, such as install ca-certificates && update-ca-certificates. When we use debian:lates or alpine:latest as the base image, /etc/ssl/certs/ca-certificates.crt already contains the latest ca certificates, and we don't need to perform an update operation every time we build.
  3. When we start mariadb with the service mariadb start command, mariadb is usually ready for service after this command ends normally and we no longer need the wait-for-it shell utility to perform the wait operation.
  4. Wrapping a large number of instructions in a CMD is usually not a wise decision. Splitting this section into a separate entrypoint script allows for a clearer CMD structure, while using exec in the entrypoint script to start the service allows the processes in the container to be associated with the container lifecycle.

Copy link
Contributor

@ComradeProgrammer ComradeProgrammer left a comment

Choose a reason for hiding this comment

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

I see the reason why you made these change, and these productive changes will be accepted.

But one more thing:

After you used 'STANDARD' as the new target of standard image, ci configuration of this repository also needs to be changed. please modify .github/workflows/build.yml as well

@atompi
Copy link
Contributor Author

atompi commented Jun 28, 2022

I see the reason why you made these change, and these productive changes will be accepted.

But one more thing:

After you used 'STANDARD' as the new target of standard image, ci configuration of this repository also needs to be changed. please modify .github/workflows/build.yml as well

Thanks for your recognition, I've pushed the changes to .github/workflows/build.yml, please review them again.

@hsluoyz
Copy link
Member

hsluoyz commented Jun 28, 2022

@ComradeProgrammer plz review

@ComradeProgrammer
Copy link
Contributor

ComradeProgrammer commented Jun 28, 2022

Thanks for your recognition, I've pushed the changes to .github/workflows/build.yml, please review them again.

Well I just tried this dockerfile and something went wrong.
When I execute docker build --target ALLINONE -t casbin/casdoor-all-in-one:latest .
I see

failed to solve with frontend dockerfile.v0: failed to create LLB definition: failed to parse stage name "ALLINONE_DB": invalid reference format: repository name must be lowercase

It seems that FROM ALLINONE_DB AS ALLINONE is not accepted

Is it because that this requires a newer version of docker?

@atompi
Copy link
Contributor Author

atompi commented Jun 29, 2022

docker build --target ALLINONE -t casbin/casdoor-all-in-one:latest .

I found a similar problem here. I am working fine with Docker CE 20.10.8 and 17.05 in Ubuntu 20.04. Perhaps there are some differences between Linux and MacOS. I've pushed the adaptability changes to PR and also added target to docker-compose.yml to use the correct stage for the build, please review them again. Thanks!

@hsluoyz hsluoyz merged commit cd902a2 into casdoor:master Jun 29, 2022
@hsluoyz
Copy link
Member

hsluoyz commented Jun 29, 2022

@atompi thanks for the contribution! Can you also help fix the Dockerfile for Casnode? https://github.com/casbin/casnode/blob/master/Dockerfile

@casbin-bot
Copy link
Contributor

🎉 This PR is included in version 1.62.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@atompi
Copy link
Contributor Author

atompi commented Jun 29, 2022

@atompi thanks for the contribution! Can you also help fix the Dockerfile for Casnode? https://github.com/casbin/casnode/blob/master/Dockerfile

Of course! I am very honored and I will finish this tomorrow.

@hsluoyz
Copy link
Member

hsluoyz commented Jun 29, 2022

Hi @atompi may I ask why you make PR to this repo? Are you applying for some program?

@atompi
Copy link
Contributor Author

atompi commented Jun 30, 2022

Hi @atompi may I ask why you make PR to this repo? Are you applying for some program?

@hsluoyz No, I didn't apply for any project, I just encountered some problems in the process of using it and solved them as I went along.

@hsluoyz
Copy link
Member

hsluoyz commented Jun 30, 2022

@atompi great! Look forward to your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants