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

Automatically retry with correct content-type #569

Merged
merged 7 commits into from
Mar 3, 2021

Conversation

trevor-vaughan
Copy link
Contributor

The podman socket will return a note about the correct content-type that
should be used for a given type of message if it is incorrect.

This is a workaround to automatically correct the content-type and
resubmit the call.

A full correction would be to update all of the different connection
entries but this appears to work correctly in the short term.

Fixes #568

@trevor-vaughan
Copy link
Contributor Author

The errors in Travis appear to be related to the busybox image and not the code changes as far as I can tell.

trevor-vaughan added a commit to trevor-vaughan/beaker-docker that referenced this pull request Jan 17, 2021
* Fixed
  * Validated support for podman in root and rootless mode
    * Requires upserve/docker-api#569
  * Changed the acceptance nodesets to centos so that testing on both
    podman and docker will function properly
  * No longer start containers in privileged mode by default for safety
    reasons
  * Updated the README
  * Fixed support for connections to socket files
  * Added a check for detection of SSH ports < 1024 if operating in
    rootless mode
  * Ensure that the IP is set to 127.0.0.1 instead of 0.0.0.0 for better
    SSH connections
  * Override host.reboot so that containers are not accidentally
    destroyed
  * Fixed PAM session entries due to known pam_loginuid issues
  * Override enable_root_login since it is already performed by
    this plugin
  * Override ssh_service retart if the init process is set as sshd since
    that causes container termination
* Added
  * Migrated to support docker-api 2.X
  * Set container names as `beaker-<uniquehash>` for easy cleanup if a
    name is not otherwise specified
  * Ensure that the underlying container object can be accessed at any
    time through `host[:docker_container]` for selective optimization
    * May want to override the archive_to and scp_to/from methods to
      call the underlying docker-api commands at some point
trevor-vaughan added a commit to trevor-vaughan/beaker-docker that referenced this pull request Jan 18, 2021
* Fixed
  * Validated support for podman in root and rootless mode
    * Requires upserve/docker-api#569
  * Changed the acceptance nodesets to centos so that testing on both
    podman and docker will function properly
  * No longer start containers in privileged mode by default for safety
    reasons
  * Updated the README
  * Fixed support for connections to socket files
  * Added a check for detection of SSH ports < 1024 if operating in
    rootless mode
  * Ensure that the IP is set to 127.0.0.1 instead of 0.0.0.0 for better
    SSH connections
  * Override host.reboot so that containers are not accidentally
    destroyed
  * Fixed PAM session entries due to known pam_loginuid issues
  * Override enable_root_login since it is already performed by
    this plugin
  * Override ssh_service retart if the init process is set as sshd since
    that causes container termination
* Added
  * Migrated to support docker-api 2.X
  * Set container names as `beaker-<uniquehash>` for easy cleanup if a
    name is not otherwise specified
  * Ensure that the underlying container object can be accessed at any
    time through `host[:docker_container]` for selective optimization
    * May want to override the archive_to and scp_to/from methods to
      call the underlying docker-api commands at some point
trevor-vaughan added a commit to trevor-vaughan/beaker-docker that referenced this pull request Jan 19, 2021
* Fixed
  * Validated support for podman in root and rootless mode
    * Requires upserve/docker-api#569
  * Changed the acceptance nodesets to centos so that testing on both
    podman and docker will function properly
  * No longer start containers in privileged mode by default for safety
    reasons
  * Updated the README
  * Fixed support for connections to socket files
  * Added a check for detection of SSH ports < 1024 if operating in
    rootless mode
  * Ensure that the IP is set to 127.0.0.1 instead of 0.0.0.0 for better
    SSH connections
  * Override host.reboot so that containers are not accidentally
    destroyed
  * Fixed PAM session entries due to known pam_loginuid issues
  * Override enable_root_login since it is already performed by
    this plugin
  * Override ssh_service retart if the init process is set as sshd since
    that causes container termination
* Added
  * Migrated to support docker-api 2.X
  * Set container names as `beaker-<hostname>-<uniquehash>` for easy
    cleanup if a name is not otherwise specified and easy identification
    when debuggin
  * Ensure that the underlying container object can be accessed at any
    time through `host[:docker_container]` for selective optimization
    * May want to override the archive_to and scp_to/from methods to
      call the underlying docker-api commands at some point
trevor-vaughan added a commit to trevor-vaughan/rubygem-simp-beaker-helpers that referenced this pull request Jan 20, 2021
* Fixed:
  * Allow all methods that can safely take SUT arrays to do so
  * Ensure thta pfact_on returns a Hash if appropriate
  * Fix container support in copy_to
* Added:
  * Explicitly support podman local and remote in copy_to

For full podman support, the following are required:
  * voxpupuli/beaker-docker#29
  * upserve/docker-api#569

SIMP-9131 #close
SIMP-9129 #close
op-ct added a commit to simp/rubygem-simp-beaker-helpers that referenced this pull request Jan 20, 2021
* Fixed:
  * Allow all methods that can safely take SUT arrays to do so
  * Ensure that pfact_on returns a Hash, if appropriate
  * Fix container support in copy_to
* Added:
  * Explicitly support podman local and remote in copy_to

For full podman support, the following are required:
  * voxpupuli/beaker-docker#29
  * upserve/docker-api#569

SIMP-9131 #close
SIMP-9129 #close

Co-authored-by: op-ct <chris.tessmer@onyxpoint.com>
@trevor-vaughan trevor-vaughan force-pushed the podman-compat branch 7 times, most recently from ddca4be to 26b7de2 Compare January 31, 2021 22:35
@trevor-vaughan
Copy link
Contributor Author

Sorry folks, the different ruby versions are being troublesome. Will update again once I have the GitHub Actions working properly.

@trevor-vaughan
Copy link
Contributor Author

Ok, this is now correctly passing in GitHub Actions and should be ready for review.

@trevor-vaughan
Copy link
Contributor Author

@phaelyn @tlunter Hi folks. Sorry for the bother, but I just wanted to check and see if you're still maintaining the gem and/or open to contributions. The PR list seems pretty extensive and hasn't really shifted much from what I can tell.

trevor-vaughan added a commit to trevor-vaughan/beaker-docker that referenced this pull request Feb 4, 2021
* Fixed
  * Validated support for podman in root and rootless mode
    * Requires upserve/docker-api#569
  * Changed the acceptance nodesets to centos so that testing on both
    podman and docker will function properly
  * No longer start containers in privileged mode by default for safety
    reasons
  * Updated the README
  * Fixed support for connections to socket files
  * Added a check for detection of SSH ports < 1024 if operating in
    rootless mode
  * Ensure that the IP is set to 127.0.0.1 instead of 0.0.0.0 for better
    SSH connections
  * Override host.reboot so that containers are not accidentally
    destroyed
  * Fixed PAM session entries due to known pam_loginuid issues
  * Override enable_root_login since it is already performed by
    this plugin
  * Override ssh_service retart if the init process is set as sshd since
    that causes container termination
* Added
  * Migrated to support docker-api 2.X
  * Set container names as `beaker-<hostname>-<uniquehash>` for easy
    cleanup if a name is not otherwise specified and easy identification
    when debuggin
  * Ensure that the underlying container object can be accessed at any
    time through `host[:docker_container]` for selective optimization
    * May want to override the archive_to and scp_to/from methods to
      call the underlying docker-api commands at some point
trevor-vaughan added a commit to trevor-vaughan/beaker-docker that referenced this pull request Feb 9, 2021
* Fixed
  * Validated support for podman in root and rootless mode
    * Requires upserve/docker-api#569
  * Changed the acceptance nodesets to centos so that testing on both
    podman and docker will function properly
  * No longer start containers in privileged mode by default for safety
    reasons
  * Updated the README
  * Fixed support for connections to socket files
  * Added a check for detection of SSH ports < 1024 if operating in
    rootless mode
  * Ensure that the IP is set to 127.0.0.1 instead of 0.0.0.0 for better
    SSH connections
  * Override host.reboot so that containers are not accidentally
    destroyed
  * Fixed PAM session entries due to known pam_loginuid issues
  * Override enable_root_login since it is already performed by
    this plugin
  * Override ssh_service retart if the init process is set as sshd since
    that causes container termination
* Added
  * Migrated to support docker-api 2.X
  * Set container names as `beaker-<hostname>-<uniquehash>` for easy
    cleanup if a name is not otherwise specified and easy identification
    when debuggin
  * Ensure that the underlying container object can be accessed at any
    time through `host[:docker_container]` for selective optimization
    * May want to override the archive_to and scp_to/from methods to
      call the underlying docker-api commands at some point
trevor-vaughan added a commit to trevor-vaughan/beaker-docker that referenced this pull request Feb 9, 2021
* Fixed
  * Validated support for podman in root and rootless mode
    * Requires upserve/docker-api#569
  * Changed the acceptance nodesets to centos so that testing on both
    podman and docker will function properly
  * No longer start containers in privileged mode by default for safety
    reasons
  * Updated the README
  * Fixed support for connections to socket files
  * Added a check for detection of SSH ports < 1024 if operating in
    rootless mode
  * Ensure that the IP is set to 127.0.0.1 instead of 0.0.0.0 for better
    SSH connections
  * Override host.reboot so that containers are not accidentally
    destroyed
  * Fixed PAM session entries due to known pam_loginuid issues
  * Override enable_root_login since it is already performed by
    this plugin
  * Override ssh_service retart if the init process is set as sshd since
    that causes container termination
* Added
  * Migrated to support docker-api 2.X
  * Set container names as `beaker-<hostname>-<uniquehash>` for easy
    cleanup if a name is not otherwise specified and easy identification
    when debuggin
  * Ensure that the underlying container object can be accessed at any
    time through `host[:docker_container]` for selective optimization
    * May want to override the archive_to and scp_to/from methods to
      call the underlying docker-api commands at some point
@tlunter
Copy link
Contributor

tlunter commented Feb 18, 2021

@trevor-vaughan can i ask why you decided to add github actions to the PR? we are already using travis for the testing of the repo.

@trevor-vaughan
Copy link
Contributor Author

@tlunter Well, the main reason is that I couldn't get them to work in Travis. I (and everyone else at this point) can guarantee that they will be run in GitHub. Once I did the work, I figured I should go ahead and submit it.

Also, I'm not sure if you were hit by this (yet) but Travis' support of FOSS projects is now severely lacking with no apparent exit plan.

If you don't like it, I can certainly back it out, but it works quite well (particularly for contributors).

@trevor-vaughan
Copy link
Contributor Author

@tlunter Would you also be OK with just leaving it and re-adding the travis stuff? You can change what you pay attention to in the project settings.

I literally couldn't get anything to run in Travis so I'd have to ping-pong it back and forth to figure out if it's still working.

@trevor-vaughan
Copy link
Contributor Author

@tlunter Do you prefer lots of little pushes with a merge/squash at the end or rebase with force push?

@tlunter
Copy link
Contributor

tlunter commented Feb 18, 2021

The former, typically with a standard merge, no squash. Would prefer squashing only when pushing to test something. Those can merged/rebased together.

@trevor-vaughan
Copy link
Contributor Author

OK, this obviously isn't making anyone's life hard but mine right now so I can fix things up this weekend if you're good with the general implementation.

I can split the github stuff into a separate commit as well if you'd like so it gets separated away from the actual code update.

@tlunter
Copy link
Contributor

tlunter commented Feb 18, 2021

That would be appreciated. We had done some work to fix the busybox issue in travis towards the end of last year, and now it's back... Going to see if I can dig up what our theory was there.

The podman socket will return a note about the correct content-type that
should be used for a given type of message if it is incorrect.

This is a workaround to automatically correct the content-type and
resubmit the call.

A full correction would be to update all of the different connection
entries but this appears to work correctly in the short term.

Updated the spec tests to use debian:stable instead of wheezy since
wheezy is no longer widely supported

Added podman-specific skips to spec tests for running on podman systems.

Explicitly use URI.open in image pulls to work around potential issues
with Ruby 2.7+.

Fixes upserve#568
Added GitHub Actions to test both the Docker and Podman code paths since
I was unable to get the Travis tests to work properly.
@trevor-vaughan
Copy link
Contributor Author

@tlunter Everything appears to be good to go now.

I went ahead and updated the rest of the tests to work with Podman and added podman tests to the GitHub Actions.

lib/docker.rb Show resolved Hide resolved
@trevor-vaughan
Copy link
Contributor Author

@tlunter Should be good to go. You might want to consider adding some of the other items in there as well. Things like version() and info() are good candidates.

@trevor-vaughan
Copy link
Contributor Author

@tlunter So, I seem to be getting some cane issues but I didn't touch anything that it's referring to. Should I just bump up the values? I didn't see any way to do line-by-line exclusions like you can in rubocop.

@tlunter
Copy link
Contributor

tlunter commented Feb 23, 2021

@trevor-vaughan I don't actually know where to look for the failures you're seeing since I only see travis on this PR, but yea, can just increase the limit for cane for now.

@trevor-vaughan
Copy link
Contributor Author

@tlunter Sorry about the flipping. Everything looks good now though. https://github.com/trevor-vaughan/docker-api/actions/runs/591109632

@trevor-vaughan
Copy link
Contributor Author

@tlunter Does this work for you now or did I miss something (pretty sure I got it all)?

lib/docker.rb Outdated Show resolved Hide resolved
lib/docker.rb Show resolved Hide resolved
@trevor-vaughan
Copy link
Contributor Author

@tlunter Well, in theory this is caching in the connection, not globally, across the board now. Unfortunately, I'm having a bit of fun with GitHub Actions not being able to decide which version of Ubuntu it wants to provide at the moment. The tests worked fine in my branch.

@trevor-vaughan
Copy link
Contributor Author

@tlunter Awesome! Is there anything else you need me to do before a merge (and hopefully release) is possible?

@tlunter tlunter merged commit dca8a0c into upserve:master Mar 3, 2021
@tlunter
Copy link
Contributor

tlunter commented Mar 3, 2021

Released as v2.1.0. Thanks a lot @trevor-vaughan

@trevor-vaughan
Copy link
Contributor Author

@tlunter Thanks for putting up with all the noise and working it through with me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The connection content_type should be modified based on the type of operation occuring
2 participants