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 IP addresses to Guacamole connection names #33

Merged
merged 15 commits into from
Feb 3, 2022

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Jan 31, 2022

πŸ—£ Description

This pull request adds private IPv4, public IPv4, and IPv6 addresses to the Guacamole connection name if such addresses exist.

πŸ’­ Motivation and context

This was requested in cisagov/cool-system-internal#43.

πŸ§ͺ Testing

Automated testing passes. I also created a new COOL staging AMI from these changes and verified that it functions as expected.

βœ… 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 new and existing tests pass.

βœ… Pre-merge checklist

  • Finalize version.

βœ… Post-merge checklist

  • Create a tag.

@jsf9k jsf9k added improvement This issue or pull request will add or improve functionality, maintainability, or ease of use version bump This issue or pull request increments the version number labels Jan 31, 2022
@jsf9k jsf9k self-assigned this Jan 31, 2022
@coveralls
Copy link

coveralls commented Jan 31, 2022

Pull Request Test Coverage Report for Build 1790836694

  • 20 of 24 (83.33%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.6%) to 76.965%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/guacscanner/guacscanner.py 19 23 82.61%
Totals Coverage Status
Change from base Build 1773474517: 1.6%
Covered Lines: 218
Relevant Lines: 265

πŸ’› - Coveralls

Use this attribute as a unique identifier for the connection instead
of the connection name.  The connection name now contains the various
IP addresses associated with the instance, so it could change without
the actual instance ID changing.

This is a better way to identify the connections associated with a
particular instance ID anyway.
@jsf9k jsf9k force-pushed the improvement/add-ip-addresses branch from 32c0f92 to a833904 Compare February 1, 2022 17:11
Query parameters must always be grouped together into a tuple, even if
there is only a single one.
@jsf9k jsf9k marked this pull request as ready for review February 2, 2022 19:17
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-love-it

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.

Straightforward and clean. I had a couple of thoughts for your consideration.

src/guacscanner/guacscanner.py Outdated Show resolved Hide resolved
src/guacscanner/guacscanner.py Outdated Show resolved Hide resolved
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
@jsf9k jsf9k force-pushed the improvement/add-ip-addresses branch 3 times, most recently from 8720b42 to 5cc9814 Compare February 3, 2022 15:44
This is to prepare for dropping support for Python 3.7, which will be
done in a future commit.

Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
jsf9k and others added 2 commits February 3, 2022 10:58
I want to use the walrus operator in a future commit, so we require at
least Python 3.8.

I've also been building a Python 3.10 Docker image in
cisagov/guacscanner-docker and running this code there, so we know
3.10 is supported.

Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
@jsf9k jsf9k force-pushed the improvement/add-ip-addresses branch from 5cc9814 to 9244182 Compare February 3, 2022 15:58
@jsf9k jsf9k merged commit 3bb235d into develop Feb 3, 2022
@jsf9k jsf9k deleted the improvement/add-ip-addresses branch February 3, 2022 17:42
jsf9k added a commit to cisagov/guacscanner-docker that referenced this pull request Jul 17, 2023
This will pick up a fewupdates from cisagov/guacscanner:
  - cisagov/guacscanner#32
  - cisagov/guacscanner#33
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 version bump This issue or pull request increments the version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants