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

Add nix flake #27

Merged
merged 29 commits into from
Sep 8, 2023
Merged

Add nix flake #27

merged 29 commits into from
Sep 8, 2023

Conversation

jasonodoom
Copy link
Contributor

@jasonodoom jasonodoom commented Aug 23, 2023

🗣 Description

This PR adds flake.nix to allow for the use of awssh without the following:

  1. Requiring Python to be installed on the host system
  2. Executing setup-env script to install awssh
  3. Managing, starting or maintaining virtual environments for Python

image

This does not require or use poetry

💭 Motivation and context

The motivation and purpose of this is to provide access to our tools quickly. Considering the recent re-provisioning of my machine I find it convenient to simply run. nix build so I can execute awssh without having to configure virtualenvs or making other system changes. This is just the beginning.

🧪 Testing

The binary was executed and I was able to successfully start a session as well as execute different awssh options. I've included a screenshot.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated to reflect the changes in this PR.

@jasonodoom jasonodoom added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label Aug 23, 2023
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

The formatting on the flake file needs to be cleaned up a bit.

flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
Co-authored-by: Shane Frasier <[email protected]>
@jasonodoom jasonodoom requested a review from jsf9k August 23, 2023 16:27
@jasonodoom jasonodoom force-pushed the add-nix-flake branch 2 times, most recently from e98fcca to 63e41d7 Compare August 23, 2023 16:45
Co-authored-by: Shane Frasier <[email protected]>

Remove whitespace

Co-authored-by: Shane Frasier <[email protected]>

Sort pakages alphabetically

Co-authored-by: Shane Frasier <[email protected]>

Add pre-commit file changes
@dav3r
Copy link
Member

dav3r commented Aug 23, 2023

@jasonodoom Please complete the "Testing" section of the PR description and explain how you tested these changes and verified that they do not negatively impact existing functionality.

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Please take a look at my suggestions, comments, and question.

flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
@dav3r
Copy link
Member

dav3r commented Aug 23, 2023

FYI, I saw that you removed the pre- and post-merge checklists from the PR template. I added them back in, since this change is worthy of a minor version bump. Please make sure that you take care of that.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mcdonnnj
Copy link
Member

FYI, I saw that you removed the pre- and post-merge checklists from the PR template. I added them back in, since this change is worthy of a minor version bump. Please make sure that you take care of that.

@dav3r Is it though? This PR does not touch the Python code so I'm not sure it should require any version modifications. We do not increment for pre-commit changes for example.

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

Given that @jasonodoom is the only one currently invested in the Nix ecosystem I think the CODEOWNERS should be updated such that the Nix paths require review from him.

@dav3r
Copy link
Member

dav3r commented Aug 23, 2023

FYI, I saw that you removed the pre- and post-merge checklists from the PR template. I added them back in, since this change is worthy of a minor version bump. Please make sure that you take care of that.

@dav3r Is it though? This PR does not touch the Python code so I'm not sure it should require any version modifications. We do not increment for pre-commit changes for example.

Gah, you are correct! I was thinking of this as a new feature, but it's just a new installation mechanism. I went ahead and removed those checklists.

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

One more tiny change request.

README.md Outdated Show resolved Hide resolved
bump_version.sh Outdated Show resolved Hide resolved
bump_version.sh Outdated Show resolved Hide resolved
bump_version.sh Outdated Show resolved Hide resolved
Co-authored-by: Shane Frasier <[email protected]>
The pre-commit linter has not yet been added in this branch, but it
will eventually be added when cisagov/skeleton-generic#143 trickles
down via Lineage.
@jasonodoom jasonodoom added the kraken 🐙 This pull request is ready to merge during the next Lineage Kraken release label Sep 6, 2023
@jsf9k jsf9k removed the kraken 🐙 This pull request is ready to merge during the next Lineage Kraken release label Sep 6, 2023
bump_version.sh Show resolved Hide resolved
@jsf9k jsf9k requested a review from dav3r September 6, 2023 15:27
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Solid work @jasonodoom! 🚀

@jsf9k
Copy link
Member

jsf9k commented Sep 7, 2023

I hate to bring this up so late, but I just noticed that Nix flakes files are experimental. I'm still OK with this PR going forward, but I figured folks should know.

README.md Outdated Show resolved Hide resolved
bump_version.sh Outdated Show resolved Hide resolved
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

LGTM ✔ :shipit:

@jasonodoom jasonodoom merged commit 62d6cd1 into develop Sep 8, 2023
25 checks passed
@jasonodoom jasonodoom deleted the add-nix-flake branch September 8, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants