-
Notifications
You must be signed in to change notification settings - Fork 4
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
set up working environment, with java #8
base: main
Are you sure you want to change the base?
Conversation
@urmishukla 👋 Because I didn't plan things all the way through 😬 when PR #11 got merged it could possibly create merge conflicts for everyone else who had a PR open. To get around this (a normal thing in development so no worries) just click the and then select the merge option. Afterwards, run
and then on your feature branch (this branch)
If you have any trouble just escape with |
Finished onboarding. |
@urmishukla I'm gone to a conference this week with limited internet access. I'll review this later this week. |
Okay, sure. Let me know if there’s anything else I can work on in the meantime! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urmishukla this is a good start 👍. In addition to the review comments I've got a few pieces of general feedback.
In Issue #3 it mentioned
Use a GitHub keyword in the body of the PR description to automatically close this Issue when the PR is merged.
At the moment this PR doesn't do that, and in addition the PR title and body (the main text area) need some improvement. The PR title should be a short description which gives an overview, though I find it useful to structure it in a similar way to how you would a good commit message — something short that tells you what it will do if you apply it / accept it. Also in general you should add some short text to the PR body (doesn't have to be long) that explains to the maintainer of the project what this PR does, why you're doing it, what Issues it might address. So you should add just a few sentences here as well. 👍
Select and install an IDE to your local machine. If you do not know or care what IDE to use, select VS Code. You are not required to use this IDE for the rest of the exercise if you would prefer to only use a text editor, but use of an IDE is a good thing to know at some level.
It would be good to document this as well. If you can also add some context on what OS you're working on that can be relevant for future information and projects.
Finally, this isn't something you need to document, but as a check when you setup your SSH keys were you able to add them to your ssh agent / keychain on your OS so that you don't need to enter your password every time you try to do a git push
? I didn't make this explicitly clear, so I thought I would check with everyone to make sure that using ssh keys isn't a painful experience.
shukla/.pre-commit-config.yaml
Outdated
# See https://pre-commit.com for more information | ||
# # See https://pre-commit.com/hooks.html for more hooks | ||
repos: | ||
- repo: https://github.com/pre-commit/pre-commit-hooks | ||
rev: v2.0.0 | ||
hooks: | ||
- id: trailing-whitespace | ||
- id: end-of-file-fixer | ||
- id: check-yaml | ||
- id: check-added-large-files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. Though, given the comment that I made #8 (comment) I'm just going to have everyone use the one that is already in the main
branch so can you delete this file?
shukla/Dockerfile
Outdated
@@ -0,0 +1,9 @@ | |||
FROM ubuntu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FROM ubuntu | |
FROM ubuntu:22.04 |
While this is valid, this is subject to breakage as you haven't specified a tag of the ubuntu
image.
shukla/Dockerfile
Outdated
apt-get -y install default-jre-headless && \ | ||
apt-get clean && \ | ||
rm -rf /var/lib/apt/lists/* | ||
RUN apt-get update && apt-get install make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This RUN
step could be combined with the previous one above it.
shukla/Dockerfile
Outdated
CMD ["make", "run"] | ||
CMD ["make", "clean"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be only one CMD
per Dockerfile, so the last one effectively clobbers the first. So at runtime only
make clean
will get run.
shukla/README.md
Outdated
7. Created Dockerfile which runs helloWorld.java. | ||
|
||
8. To build this Dockerfile, execute the following build commands: | ||
A) sudo docker image build -t urmishukla/ubuntu-java:v1 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A) sudo docker image build -t urmishukla/ubuntu-java:v1 . | |
A) sudo docker build -t urmishukla/ubuntu-java:v1 . |
while not technically wrong, you're not going to see this syntax very often and docker build
is fine. If you need to use sudo
here then it seems that you haven't added yourself and docker to the proper user groups to avoid this. Do you know how to do this?
shukla/README.md
Outdated
8. To build this Dockerfile, execute the following build commands: | ||
A) sudo docker image build -t urmishukla/ubuntu-java:v1 . | ||
B) run "sudo docker image ls" to check that this new Docker image is present. | ||
C) rsudo docker container run --mount type=bind,source=${PWD},target=/temp urmishukla/ubuntu-java:v1 java/temp/helloWorld.java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of trying to bind mount the files in here (which is sometimes a useful approach) try to COPY
the relevant files that you need into the Docker image when you're defining the Dockerfile. This is a more common approach as then the built Docker image is a binary that has the executable inside of it, which can then be distributed.
@urmishukla just checking in to see if you have questions. Is there anything you want to discuss that is currently blocking you? |
@matthewfeickert Hi Matthew! So far, no. I've begun implementing some of these changes, but might be busy until Wednesday since I have midterms all week until then. I can probably update my pull request by Thursday afternoon or so, if that works |
SGTM! 👍 Hope that your midterms go great! |
Hi Matthew, I made all the changes you mentioned, but for some reason my Dockerfile won't correctly run my program. When I type "make build", "make run", and "make clean" into the shukla directory, my java program runs. But the Dockerfile I have gives me the error: "make: *** No rule to make target 'clean'. Stop" |
Hi Matthew, in the meantime, should I begin to work through the Intermediate Research Software Development in Python module? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @urmishukla. My apologies that we haven't touched base in a very long time — this is due to me having too many conflicts of time come up and is my fault and not the result of anything you did.
should I begin to work through the Intermediate Research Software Development in Python module?
I think these fixes shouldn't take long to implement, so yeah starting work on that module would be a good idea. It should all be open and public now, but if for some reason you're paywalled let me know.
shukla/Dockerfile
Outdated
apt-get update && apt-get install make | ||
|
||
CMD ["make", "build"] && ["make", "run"] && ["make", "clean"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiling is an expensive operation which you don't want to have to do at runtime. It would be better to compile the program while building the image and then only run it at container runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urmishukla This suggestion still need to be addressed.
shukla/README.md
Outdated
@@ -0,0 +1,22 @@ | |||
1. Forked repository, cloned onto local device | |||
|
|||
2. Download VSCode for Ubuntu Virtual Machine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along the lines of #8 (review) it would be good to get a bit more information on what the IDE setup is like. Linking to existing docs from VS Code is good too.
.pre-commit-config.yaml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to delete the existing .pre-commit-config.yaml
file, but just use it instead of making a new one. So you'll want to restore this file to the state of origin/main
.
Co-authored-by: Matthew Feickert <[email protected]>
Co-authored-by: Matthew Feickert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urmishukla thanks for adding the suggested changes that I had left in the last review. There are still parts of the last review that weren't addressed though, so please look at those.
If you have questions on what do to or how to address them please ask.
.pre-commit-config.yaml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urmishukla as I mentioned in #8 (comment) you don't want to delete the file altogether.
So just
git fetch --all --prune
git checkout upstream/main -- .pre-commit-config.yaml # Assuming you named the remote that you forked from "upstream"
and add that back in.
shukla/Dockerfile
Outdated
apt-get update && apt-get install make | ||
|
||
CMD ["make", "build"] && ["make", "run"] && ["make", "clean"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urmishukla This suggestion still need to be addressed.
👋 Hi @urmishukla. Did you have any questions on my comments from my previous review? |
Development environment setup for Urmi Shukla at the DSI. Outlines the entire process of setting up small dev environment, building Dockerfile and running a small helloWorld.java program.
Closes #3