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

First commits #3

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

First commits #3

wants to merge 17 commits into from

Conversation

king-alexander
Copy link
Collaborator

@king-alexander king-alexander commented Apr 18, 2023

🗣 Description

These first commits modify cisagov/skeleton-ansible-role to install The Admiral. The changes group initial code, files, and documentation for the role so we can reuse them across deployments.

💭 Motivation and context

We want to use this role to ship the Admiral off to sea. See cisagov/cyhy_amis#639 for full details of the voyage.

🧪 Testing

I updated the default test scenario in 125e9eb. All 33 tests passed (in 19.34 seconds).

My testing environment was pyenv 2.3.15 running Python 3.9.13.

In addition, I successfully used the role to create the /var/cyhy/admiral directory on my local machine and verified that the correct Docker Compose files were in place.

✅ 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.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

@king-alexander king-alexander added documentation This issue or pull request improves or adds to documentation improvement This issue or pull request will add or improve functionality, maintainability, or ease of use labels Apr 18, 2023
@king-alexander king-alexander marked this pull request as ready for review April 18, 2023 16:10
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.

Strong work!

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.

This all looks great, aside from the minor things I noted, but before I approve this, I'd really like to see the "Testing" section mention that you successfully used this role to install the Admiral somewhere and that it functioned as expected.

meta/main.yml Outdated Show resolved Hide resolved
meta/main.yml Show resolved Hide resolved
@king-alexander
Copy link
Collaborator Author

This all looks great, aside from the minor things I noted, but before I approve this, I'd really like to see the "Testing" section mention that you successfully used this role to install the Admiral somewhere and that it functioned as expected.

Functions as expected!

@jsf9k
Copy link
Member

jsf9k commented Apr 20, 2023

@king-alexander - Reminder to turn on the branch protections before or after you merge this one (or ask me to do it, if you lack the necessary permissions).

Copy link
Member

Choose a reason for hiding this comment

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

I don't know that this file belongs in this project. In a deployment there is no reason for the Admiral source code to be on the instance. Any development work would involve pulling down a built Docker image with the changes being worked on and that would be done using the appropriate tag from Docker Hub.

Copy link
Collaborator Author

@king-alexander king-alexander Apr 21, 2023

Choose a reason for hiding this comment

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

Removed in b390329.

You bring up a good point. The development configuration exposes a bash shell which runs the script to collect new certificates. Should that be shoved into docker-compose, or do we need to rethink how to probe the CT Search API?

Copy link
Member

Choose a reason for hiding this comment

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

That is a good question. When does that script need to be run? It might make sense to create an issue to incorporate that as a docker-compose.override.yml configuration option if it would see regular use.

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 run the script every time I boot up the Admiral composition (on average, about once per week). I created cisagov/admiral-docker#13 to capture how we manage these Compose files as a unit of work.

Copy link
Member

Choose a reason for hiding this comment

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

@mcdonnnj - Can this conversation be resolved?

meta/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
molecule/default/tests/test_default.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@king-alexander king-alexander force-pushed the first-commits branch 2 times, most recently from df4546d to 5fff4cb Compare April 25, 2023 14:47
@king-alexander king-alexander self-assigned this Apr 25, 2023
README.md Outdated Show resolved Hide resolved
@dav3r
Copy link
Member

dav3r commented Apr 26, 2023

@king-alexander Why are the molecule tests failing?

@mcdonnnj
Copy link
Member

@king-alexander Why are the molecule tests failing?

@dav3r cisagov/skeleton-ansible-role#139

@dav3r
Copy link
Member

dav3r commented May 15, 2023

@king-alexander Why are the molecule tests failing?

@dav3r cisagov/skeleton-ansible-role#139

@king-alexander Any updates here now that cisagov/skeleton-ansible-role#139 has been merged?

.github/CODEOWNERS Outdated Show resolved Hide resolved
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.

Looks like a winner now- good work! ⚓

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.

Approval intensifies!!!

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 comment and one change request.

.github/CODEOWNERS Show resolved Hide resolved
meta/main.yml Show resolved Hide resolved
@king-alexander
Copy link
Collaborator Author

Approval reaches a fever pitch!!!

Let's merge #9 before we merge this one.

I rebased now that #9 is merged. Note that I dropped the commits where I disable and reenable Amazon Linux 2023 now that the platform works just fine.

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.

I approve with the power of 1000 suns!!!

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.

I noticed two Docker images that should be updated here, assuming they work. Please check them out.

files/docker-compose.yml Outdated Show resolved Hide resolved
files/docker-compose.yml Outdated Show resolved Hide resolved
@king-alexander
Copy link
Collaborator Author

I noticed two Docker images that should be updated here, assuming they work. Please check them out.

Your assumptions were correct. I noticed no issues with either image.

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.

Approval intensifies! 🎬

@king-alexander
Copy link
Collaborator Author

Rebase after latest pull request is merged, then send up flare 🎇

king-alexander and others added 11 commits June 27, 2024 11:50
These composition files will be deployed in the Ansible role.
This role executes 3 tasks: creating the `/var/cyhy/admiral` directory,
installing the Docker Compose configuration, and installing the Docker
Compose development configuration.
We verify that the correct directory exists with the correct files and
permisions.
Update the README from the generic skeleton to details specific to this
project. There are no requirements or role variables, but the project
does depend on the Ansible Role for Docker.
Future development work should be done from a pre-built Docker image, so
the development configuration does not belong in this project.
Co-authored-by: dav3r <[email protected]>
king-alexander and others added 6 commits June 27, 2024 13:19
Alphabetize owners and add myself as an owner for the .github directory.
These two variables provide the option to set ownership for any files or
directories created by the role. I updated the README to reflect this
new ability.
Use the role variables to set ownership for the Docker composition in
addition to the admiral directory.
@king-alexander
Copy link
Collaborator Author

king-alexander commented Jun 27, 2024

Rebase after latest pull request is merged, then send up flare 🎇

Flare burns brighter! 🎆 @dav3r @jsf9k @mcdonnnj @felddy

@jsf9k
Copy link
Member

jsf9k commented Jun 27, 2024

Rebase after latest pull request is merged, then send up flare 🎇

Flare burns brighter! 🎆 @dav3r @jsf9k @mcdonnnj @felddy

image

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.

Approval shines bright like a 💎

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.

And Rohan @jsf9k will answer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This issue or pull request improves or adds to documentation 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.

Restrict Redis to localhost
4 participants