openedx / openedx-atlas

An Open edX CLI tool for moving translation files from openedx-translations
GNU Affero General Public License v3.0
3 stars 7 forks source link

feat: install shellcheck, shellspec and gengetoptions via Makefile #7

Closed OmarIthawi closed 1 year ago

OmarIthawi commented 1 year ago

This PR is a nice-to-have feature in preparation to completing the remaining features of atlas. It's useful to avoid manual installation of requirements.

Changes

References

This pull request is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.

Check the links above for full information about the overall project.

openedx-webhooks commented 1 year ago

Thanks for the pull request, @OmarIthawi! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

brian-smith-tcril commented 1 year ago

The only concern I have here is that this is opinionated about how/where to install shellcheck/shellspec/getoptions

I think it should be fine, but it'd be nice to see if anyone else has opinions on this.

OmarIthawi commented 1 year ago

The only concern I have here is that this is opinionated about how/where to install shellcheck/shellspec/getoptions

Good note @brian-smith-tcril. It's a bit opinionated.

I'm stealing a bit from a Python virtualenv pattern in Makefile that I've found somewhere on GitHub.

I think the place is fine since the target audience for this Makefile is the atlas developers (editors) and not the users (fork maintainers, translators, etc). The good thing is that it's all within the repository, so not much side effects to the developer environment.

What's the drawback you think of installing those binaries in the openedx-atlas/.bin directory?

brian-smith-tcril commented 1 year ago

What's the drawback you think of installing those binaries in the openedx-atlas/.bin directory?

I tend to prefer having things installed via a package manager. Once these binaries have been installed to openedx-atlas/.bin and .bin_is_ready has been created, there's no process for automatically updating the binaries.

I don't think there are any major security implications to not having shellcheck/shellspec/getoptions updated, but it's a pattern I would usually avoid.

That being said, I can see the benefit of not needing to manually install these.

OmarIthawi commented 1 year ago

I tend to prefer having things installed via a package manager. Once these binaries have been installed to openedx-atlas/.bin and .bin_is_ready has been created, there's no process for automatically updating the binaries.

We're definity pinning the binaries to stabilize the experience. Much like we do with requirements.txt, but you're right, we're missing on the automatic updates like we get with local pip-tools or cloud-based @dependabot.

I don't think there are any major security implications to not having shellcheck/shellspec/getoptions updated, but it's a pattern I would usually avoid.

I understand the point. This pull request isn't blocking anything, and more of a nice to have. I'm fine with having it closed or merged ¯_(ツ)_/¯

OmarIthawi commented 1 year ago

I'm closing this PR for now. I have my binaries installed in ~/bin and it works just as fine.

openedx-webhooks commented 1 year ago

@OmarIthawi Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.