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

[SHIR] Install Microsoft JDK and then delete msi to decrease image size #17

Merged
merged 2 commits into from Sep 11, 2023

Conversation

missingcharacter
Copy link
Contributor

@missingcharacter missingcharacter commented Aug 8, 2023

Included changes:

  • [SHIR] Install Microsoft JDK and then delete msi to decrease image size
  • [SHIR] Remove SHIR msi to decrease image size

@missingcharacter
Copy link
Contributor Author

@microsoft-github-policy-service agree

@missingcharacter
Copy link
Contributor Author

BTW, I tested the resulting image ghcr.io/missingcharacter/adf-shir:PR-1.6.57b4dde myself and it works, I am able to register a node and able to create parquet files

@jikuja
Copy link

jikuja commented Aug 13, 2023

BTW, I tested the resulting image ghcr.io/missingcharacter/adf-shir:PR-1.6.57b4dde myself and it works, I am able to register a node and able to create parquet files

Which container orchestration did you use?

Copy link

@jikuja jikuja left a comment

Choose a reason for hiding this comment

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

Nice additions and JRE/JDK installation was something to add before running this in any production workload.


Related ticket: MicrosoftDocs/azure-docs#113486

I did not add any comments for https://github.com/Azure/Azure-Data-Factory-Integration-Runtime-in-Windows-Container/pull/17/files#diff-38ab18146fe7ed8ca5f34789d90179e6212e48f778045320c3a129d18a456849R57-R59

Documentation is really bad but from SO questions, some parts of the documentation and error messages we can see that SHIR tries first to use registry keys and than JAVA_HOME to locate java.dll. Nothing suggests that PATH changes are needed for SHIR. If usage of $JavaHome can be removed from build.ps1 it will significantly simplify required logic for build-time java installer selection.

.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
SHIR/build.ps1 Outdated Show resolved Hide resolved
SHIR/build.ps1 Outdated Show resolved Hide resolved
@missingcharacter
Copy link
Contributor Author

Which container orchestration did you use?

@jikuja , I tested on Azure App Service on a Service Plan ith Hyper-V: true

@byran77
Copy link
Collaborator

byran77 commented Sep 5, 2023

Hi @missingcharacter,
nice points to improve. It would be better to separate the changes into different PRs.

@jikuja
Copy link

jikuja commented Sep 5, 2023

I have been re-iterating this idea on my head...

Would it be possible to do following:

  • create SHIR/scripts/.placeholder
  • move code that installs JVM into contrib/01-install-jvm.ps1
  • modify build.ps1 to execute .ps1 files SHIR/scripts
  • document this

This would make easier to extend capabilities by adding files just before build process.

@byran77
Copy link
Collaborator

byran77 commented Sep 6, 2023

I have been re-iterating this idea on my head...

Would it be possible to do following:

  • create SHIR/scripts/.placeholder
  • move code that installs JVM into contrib/01-install-jvm.ps1
  • modify build.ps1 to execute .ps1 files SHIR/scripts
  • document this

This would make easier to extend capabilities by adding files just before build process.

lgtm
It is reasonable to keep only the basic building logic in build.ps1 for better maintainability.

@missingcharacter
Copy link
Contributor Author

@byran77 I moved GitHub Action to its own PR #19

@missingcharacter
Copy link
Contributor Author

@byran77 I've updated this PR too

@byran77
Copy link
Collaborator

byran77 commented Sep 8, 2023

thanks @missingcharacter, it looks good.
One more thing is to add some documents on the building argument INSTALL_JDK

@missingcharacter
Copy link
Contributor Author

@byran77 I added docs

@byran77 byran77 merged commit b4989a0 into Azure:main Sep 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants