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

Docker based development for Jekyll #7273

Closed
wants to merge 16 commits into from

Conversation

epugh
Copy link
Collaborator

@epugh epugh commented May 30, 2024

Description

Frustrated with Ruby install issues? Using Docker already? This let's you contribute documentation improvements without installing Ruby locally.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

Checklist

  • [ x] By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@epugh epugh marked this pull request as draft May 30, 2024 16:15
@Naarcha-AWS
Copy link
Collaborator

Love this @epugh! Let me know when it's ready for review.

@Naarcha-AWS Naarcha-AWS added the 2 - In progress Issue/PR: The issue or PR is in progress. label May 30, 2024
leanneeliatra and others added 4 commits May 30, 2024 14:59
…5782 (opensearch-project#7113)

* adding top ten security best practices

Signed-off-by: [email protected] <[email protected]>

* changing nav order

Signed-off-by: [email protected] <[email protected]>

* adding to best practices

Signed-off-by: AntonEliatra <[email protected]>

* adding to best practices

Signed-off-by: AntonEliatra <[email protected]>

* adding to best practices

Signed-off-by: AntonEliatra <[email protected]>

* adding bonus tip

Signed-off-by: [email protected] <[email protected]>

* updates to best practices

Signed-off-by: [email protected] <[email protected]>

* integrating Darshits suggestions for improvement and reviewdog fixes

Signed-off-by: [email protected] <[email protected]>

* review suggestions to grammer

Signed-off-by: [email protected] <[email protected]>

* review suggestions to grammer

Signed-off-by: [email protected] <[email protected]>

* review suggestions to grammer

Signed-off-by: [email protected] <[email protected]>

* review suggestions to grammer

Signed-off-by: [email protected] <[email protected]>

* review suggestions to grammer

Signed-off-by: [email protected] <[email protected]>

* reviewdog update

Signed-off-by: [email protected] <[email protected]>

* Apply suggestions from code review

Co-authored-by: Naarcha-AWS <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>

* reviewdog updates

Signed-off-by: [email protected] <[email protected]>

* Update _security/configuration/best-practices.md

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>

* Update best-practices.md

Signed-off-by: AntonEliatra <[email protected]>

* Update best-practices.md

Signed-off-by: AntonEliatra <[email protected]>

* Add editorial comment

Signed-off-by: Naarcha-AWS <[email protected]>

* Update best-practices.md

Signed-off-by: AntonEliatra <[email protected]>

* Update _security/configuration/best-practices.md

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>

* Update best-practices.md

Signed-off-by: AntonEliatra <[email protected]>

* Update best-practices.md

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>

---------

Signed-off-by: [email protected] <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>
Co-authored-by: AntonEliatra <[email protected]>
Co-authored-by: Naarcha-AWS <[email protected]>
Co-authored-by: Nathan Bower <[email protected]>
* Explain k in approximate k-NN

Signed-off-by: Fanit Kolchina <[email protected]>

* Additional info

Signed-off-by: Fanit Kolchina <[email protected]>

* Delete engine row in table

Signed-off-by: Fanit Kolchina <[email protected]>

* Add a clarification to the table

Signed-off-by: Fanit Kolchina <[email protected]>

* Apply suggestions from code review

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: kolchfa-aws <[email protected]>

* Update _search-plugins/knn/approximate-knn.md

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: kolchfa-aws <[email protected]>

---------

Signed-off-by: Fanit Kolchina <[email protected]>
Signed-off-by: kolchfa-aws <[email protected]>
Co-authored-by: Nathan Bower <[email protected]>
@dtaivpp
Copy link
Contributor

dtaivpp commented May 30, 2024

Ping

@epugh
Copy link
Collaborator Author

epugh commented May 30, 2024

Hi all.. So I paired on this with @dtaivpp and I think I have something working. One potentially controversial decision was to lock the version of Ruby to 3.2.4. I polled my local Ruby group on their opinions on if the Gemfile.lock should be checked in and the consensus was that yes, it should be, and yes, you should pick a Ruby and stick with it. By doing this, we don't regenerate the Gemfile.lock constantly, and we can upgrade dependencies more intentionally. I would LOVE to hear if this works properly on a Linux and Window's environments....

@dtaivpp
Copy link
Contributor

dtaivpp commented May 30, 2024

@epugh was the --net=host the solution for your use case? You think we should document that pattern?

@epugh
Copy link
Collaborator Author

epugh commented May 30, 2024

@epugh was the --net=host the solution for your use case? You think we should document that pattern?

I actually didn't a get chance to try that one.....

Copy link
Contributor

@dtaivpp dtaivpp left a comment

Choose a reason for hiding this comment

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

Works on Linux but I had a few comments to make it more usable.


# Enable the link checker
ENV JEKYLL_LINK_CHECKER="internal"

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move the host and jekyll_env to an env variable? This way we can allow the users to more easily set variables.

ENV JEKYLL_ENV=development
ENV HOST=127.0.0.1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably! I haven't actually used ENV much in the past, so it felt "new" to me... ! Please make the change!

Dockerfile Outdated
# Enable the link checker
ENV JEKYLL_LINK_CHECKER="internal"

CMD ["bundle", "exec", "jekyll", "serve", "--host", "0.0.0.0", "--port", "4000", "--incremental", "--livereload", "--trace"]
Copy link
Contributor

Choose a reason for hiding this comment

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

The other thing is maybe at the end of this we add config files for overwriting the site.url.

--config _config.yml,_config_docker.yml

When I was testing I found the only way to access the site off my machine was using a _config_docker.yml with the following contents and the JEKYLL_ENV=production.

url: "http://10.0.5.93:4000"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I love it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should we add a 1 liner description in the CONTRIBUTING.md file ? "To run this on your droplet..."???

Copy link
Contributor

Choose a reason for hiding this comment

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

100% I'll add that along with a bit more about the pattern.

1. Environment now is defaulted to run on localhost
2. Added env var's to run in a hosted manner
3. Added documentation to run a demo server hosted

Signed-off-by: David Tippett <[email protected]>
Copy link
Contributor

@dtaivpp dtaivpp left a comment

Choose a reason for hiding this comment

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

@kolchfa-aws I reviewed these and confirmed the steps outlined work and are documented correctly!

@Naarcha-AWS
Copy link
Collaborator

Test on Windows was successful.

@Naarcha-AWS
Copy link
Collaborator

@epugh: Once you take this out of draft mode, I'll give the language and formatting a proper review. Ping me when you're ready.

@epugh epugh marked this pull request as ready for review May 31, 2024 21:21
@epugh
Copy link
Collaborator Author

epugh commented May 31, 2024

@Naarcha-AWS it's no longer draft! THanks for testing on windows!

@kolchfa-aws
Copy link
Collaborator

Thanks for validating the steps, @dtaivpp! Much appreciated. Let's get a review from @AMoo-Miki too (specifically fixing the Ruby version).

Copy link
Collaborator

@Naarcha-AWS Naarcha-AWS left a comment

Choose a reason for hiding this comment

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

This changes suggested make the formatting more in line with the next section.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@Naarcha-AWS
Copy link
Collaborator

@epugh: Per the advice of @AMoo-Miki, the gemfile shouldn't prevent someone from using a different version of ruby if they so choose. One alternative is to make a gemfile specific for Docker.

@hdhalter
Copy link
Contributor

@dtaivpp , @epugh - Is Miki's comment a blocker for pushing this up?

@dtaivpp
Copy link
Contributor

dtaivpp commented Jul 11, 2024

@hdhalter I think that comes down to the preferences of the project. It's pretty standard to standardize on a single version of ruby. That way changes that are made in one persons environment should be guaranteed to work in production with the same ruby version.

@AMoo-Miki I'm curious what advantage there is to allowing users to use any ruby version they'd like. That feels like a risk

Eric is on vacation for a few weeks, so I accepted your changes, Nate.

Co-authored-by: Naarcha-AWS <[email protected]>
Signed-off-by: Heather Halter <[email protected]>
@epugh
Copy link
Collaborator Author

epugh commented Aug 22, 2024

Just went to add some docs and ran build.sh and hit the

Could not find jekyll-last-modified-at-1.3.0, i18n-1.14.3, rouge-4.2.0, posix-spawn-0.3.15, public_suffix-5.0.4, concurrent-ruby-1.2.3, sass-embedded-1.71.1-x86_64-darwin, ffi-1.16.3, google-protobuf-3.25.3-x86_64-darwin, rb-inotify-0.10.1 in locally installed gems

message.

The big change that impacts non Docker users in this set up is that we check into the repo Gemfile.lock and specify the version of Ruby to use, instead of leaving it open.... So you have to use a specific Ruby... Though we will then know all the versions of the gems you use ;-). This is my preference.

If that is too much, I could look at having Docker install ruby and the gems at start up time. That would make firing up the docker image a lot slower, but might be a more gentle introduction of Docker into the tool chain.... Thoughts @AMoo-Miki ?

@epugh
Copy link
Collaborator Author

epugh commented Oct 16, 2024

This was solved in #8220! Thanks @hainenber for getting this problem solved!

@epugh epugh closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In progress Issue/PR: The issue or PR is in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants