Skip to content
This repository was archived by the owner on Mar 20, 2021. It is now read-only.

Removed absolute path usage from _head.html #102

Merged
merged 4 commits into from
Mar 16, 2020
Merged

Removed absolute path usage from _head.html #102

merged 4 commits into from
Mar 16, 2020

Conversation

dellagustin
Copy link
Contributor

@dellagustin dellagustin commented Feb 23, 2020

This PR partly addresses #115

Remarks

When trying to run the command `jekyll serve` using `_config_dev.yml`,
it failed with the following error:
  Conversion error: Jekyll::Converters::Scss encountered an error while
  converting 'assets/css/atom.scss': undefined method `to_sym' for true:TrueClass Did
you mean? to_s
------------------------------------------------
Jekyll 4.0.0   Please append `--trace` to the `serve` command
               for any additional information or backtrace.
------------------------------------------------
This happens because in `_config_dev.yml` sourcemap is set to `true`,
while the documentation at
https://github.com/jekyll/jekyll-sass-converter stated the allowed
values are `never`, `always` and `development`.
This commit enables the usage of Docker for setting up a development
environment.

It is an implementation for
InnerSourceCommons/innersourcecommons.org#91

With this development, with the command `docker-compose up`, a docker
image is built and a docker contain is run serving the webpage, that
can be accessed on https://localhost:4000

Files:
- `.dockerignore` - ensures that only the necessary files are used in
the context passed to the docker image build, saving build time
- `Dockerfile` - The commands used to build a docker image
- `docker-compose.yml` - Configuration for docker-compose, that enables
a simple development environment setup, automating the image build and
container orchestration

Known limitations:
- Website is not rebuilt automatically when a file is changed, requiring
a container restart (this was expected to work out of the box with
`jekyll serve`, but testing showed it did not work)
- Docker Toolbox is not supported - with this version of docker, the
page can be loaded, but assets are not loaded, as the browser tries to
fetch them from http://0.0.0.0, and the website must be accessed on the
IP returned by `docker-machine ip`. More investigation is necessary.

Explanations:
- `RUN apk add --no-cache g++ gcc make musl-dev` - necessary for
building gem native extensions (eventmachine) - Error message "You have
to install development tools first."
- `RUN bundle update --bundler` - Image build on Docker Toolbox worked
without this, but when trying to build the image on Docker for Mac, it
raised the following error: `find_spec_for_exe': Could not find
'bundler' (1.16.1) required by your /InnerSource/Gemfile.lock.
(Gem::GemNotFoundException)
To update to the latest version installed on your system, run `bundle
update --bundler`.
- `--host 0.0.0.0` - without this line, jekyll will only accept traffice
incoming from `localhost` or `127.0.0.1`, which is not the case when
using Docker for Mac or Docker Toolbox, as it forwards traffic from the host to the
container. Without this line, the browser will show errors
ERR_CONNECTION_REFUSED (Toolbox) or ERR_EMPTY_RESPONSE (Mac).

Remarks:
- Only tested on Docker for Mac and Docker Toolbox

References:
- https://hub.docker.com/_/ruby
- https://docs.docker.com/engine/reference/builder/#dockerignore-file
- https://docs.docker.com/engine/reference/builder/
- docker-library/ruby#163 (How to install
eventmachine)
-
https://stackoverflow.com/questions/41485217/mount-current-directory-as-a-volume-in-docker-on-windows-10
-
https://stackoverflow.com/questions/42893475/how-to-run-jekyll-serve-pointing-to-another-directory
-
https://stackoverflow.com/questions/50540721/docker-toolbox-error-response-from-daemon-invalid-mode-root-docker
-
https://medium.com/@Charles_Stover/fixing-volumes-in-docker-toolbox-4ad5ace0e572
-
https://sashabrava.github.io/2018/making-Jekyll-available-on-local-network.html
- microsoft/vscode-remote-release#764
-
https://stackoverflow.com/questions/16608466/connect-to-a-locally-built-jekyll-server-using-mobile-devices-in-the-lan/16608698
@rrrutledge
Copy link
Collaborator

Awesome work!

Is it possible to see this change deployed to GitHub pages before merge so as to very that things still work?

@lenucksi
Copy link
Member

lenucksi commented Feb 25, 2020

Awesome work!

Is it possible to see this change deployed to GitHub pages before merge so as to very that things still work?

The changes built on CI which means that Jekyll didn't crash and burn.
If the site looks the same is something that would need to be validated by yourself locally, with a stage deployment or some Selenium excess.

Stage deployment would be a second, non-public deployment e.g. using Netlify and a second branch, in other words:

  • You would first pull request against our staging branch that would be deployed do the non-public deployment
  • and then upon acceptance of the result merge the staging branch into the master with a second pull request that triggers the deployment to the public website.

@dellagustin-sap
Copy link
Contributor

@rrrutledge that could be done manually, but I would be the equivalent of merging it and letting the build pipeline deploy it.

Reviewing it locally and having exactly the same experience you have when you deploy to the productive website is virtually impossible.

We could try to increase test coverage, we would have to be very smart about it, not to commit to stuff we are not sure we want to keep, otherwise, when you refactor code, you also have to refactor test code.

@lenucksi
Copy link
Member

Reviewing it locally and having exactly the same experience you have when you deploy to the productive website is virtually impossible.

I would argue that in the current state of things a manual and local preview will be sufficient in nearly all cases. It if breaks, we can go back very fast. It's not like we're operating the Google front page including search with that website in my opinion. ;)
I wouldn't complain about having an automated staging deployment though. :D

@dellagustin
Copy link
Contributor Author

@lenucksi I am of the very same opinion.

@rrrutledge
Copy link
Collaborator

Sounds good. Just check the altered links after deployment?

@lenucksi
Copy link
Member

lenucksi commented Mar 5, 2020

When should we merge this? Once all the checkboxes are ticked?

@dellagustin
Copy link
Contributor Author

@rrrutledge

Sounds good. Just check the altered links after deployment?
Sure

@lenucksi

When should we merge this? Once all the checkboxes are ticked?

Good question, what happens if you merge now and I push another commit to the branch that originated this PR?
Would it be reopened? Is that an acceptable GitHub workflow? I never did it like this, but it "smells bad".

Maybe I should create an issue with the title "Refactor: reduce usage of absolute paths", transfer the checkboxes there and we go merging bite size PRs (as this one) with each individual correction.

Any opinions?

@rrrutledge
Copy link
Collaborator

Having an issue sounds great!

@dellagustin dellagustin changed the title Refactor: reduce usage of absolute paths Removed absolute path usage from _head.html Mar 7, 2020
@lenucksi
Copy link
Member

@lenucksi

When should we merge this? Once all the checkboxes are ticked?

As soon as we can, i.e. as soon as the merge-conflict that exists currently is handled.

Good question, what happens if you merge now and I push another commit to the branch that originated this PR?

A cute baby kitten dies. ;)

Would it be reopened? Is that an acceptable GitHub workflow? I never did it like this, but it "smells bad".

No, I don't think so. No. Yes it does in my book too. ;)

Maybe I should create an issue with the title "Refactor: reduce usage of absolute paths", transfer the checkboxes there and we go merging bite size PRs (as this one) with each individual correction.

Any opinions?

Sounds fine to me. Please close this PR if you're going to split it the things up. If you reference this PR in the new PRs that would be very useful :D
I'm all for small PRs. Most of the times.

@lenucksi lenucksi merged commit ef3b067 into InnerSourceCommons:master Mar 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants