search

saltstack-formulas / docker-formula

Install and set up Docker
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
136 stars 330 forks source link

fix(clean): fix syntax error with use_upstream repo #285

Closed danny-smit closed 3 years ago

danny-smit commented 3 years ago

With use_upstream: repo, several errors occur:

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

This fixes the following errors when use_upstream: repo is used:

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

I attempted to add tests for the use_upstream: repo case, but it seems that it is not supported on all platforms that are included in the CI tests. So I'm not sure how to deal with that, some advice on that would be welcome.

myii commented 3 years ago

@danny-smit This comment isn't directly related to the PR itself but your status as a First-time contributor, even though you've had PRs merged to this formula. Since GitHub has just introduced this new "feature" of First-time contributors need a maintainer to approve running workflows, we're going to keep needing manual intervention each time.

I've had a brief look and your commit identity doesn't show your profile picture. I'm assuming that the e-mail addresses for your commits and your GitHub account don't match up. If I'm right, it seems that GitHub doesn't do a good job of keeping track of contributors in this case.

I'm not asking you to make an adjustment, since you really shouldn't have to. Rather, just a confirmation that this is the case, so that we can consider following this up with GitHub Support.

myii commented 3 years ago

I attempted to add tests for the use_upstream: repo case, but it seems that it is not supported on all platforms that are included in the CI tests. So I'm not sure how to deal with that, some advice on that would be welcome.

@danny-smit With respect to this, InSpec controls can be run for specific platforms only, using only_if. Here are some examples across the formulas:

noelmcloughlin commented 3 years ago

using only_if. @myii nice!! I like it.

danny-smit commented 3 years ago

I've had a brief look and your commit identity doesn't show your profile picture. I'm assuming that the e-mail addresses for your commits and your GitHub account don't match up. If I'm right, it seems that GitHub doesn't do a good job of keeping track of contributors in this case.

@myii Yes, I just checked and you are correct. The e-mail addresses didn't match.

@danny-smit With respect to this, InSpec controls can be run for specific platforms only, using only_if. Here are some examples across the formulas:

Thanks, I'll have a look at that.

danny-smit commented 3 years ago

I've added a test to cover the 'repo' scenario.

However some of the kitchen instances do not seem to work locally for me (even without my own changes), so it is difficult to be sure if it works for all OS'es without using the GitLab CI pipeline.

myii commented 3 years ago

However some of the kitchen instances do not seem to work locally for me (even without my own changes), so it is difficult to be sure if it works for all OS'es without using the GitLab CI pipeline.

@danny-smit If they're not working locally, that's usually an issue in the environment that Kitchen is running in (normally the gem versions). You can take advantage of the Gemfile.lock to ensure the exact gems are installed to get things working properly. Feel free to share the errors you're facing -- perhaps we can help figure things out.

In terms of the pipelines here, then that's what they were created for! Please take full advantage of them to run your tests. The only problem we're now facing is that GitHub Actions won't always run automatically (as we discussed above). So someone has to come along and hit the button to allow it to run (for "first-time contributors"). GitLab CI doesn't face that problem, thankfully -- but this is one of the only formulas that doesn't work too well in GitLab, which is why we had to use Actions instead.

Anyway, for this specific PR, you should be able to keeping pushing changes and the CI should run automatically.

danny-smit commented 3 years ago

@myii Thanks, yesterday I was having issues with the CentOS instances, which reported that the docker services were not running. When stepping into the containers, the services seem to be running just fine. It looked like the ssh connection to the container was failing. However I'm not really sure, because today bintray has shutdown their services which is used in all the tests. The tests don't even get to the point where it failed yesterday, so I can't really test it properly at the moment (caused by https://github.com/saltstack-formulas/docker-formula/issues/281)

danny-smit commented 3 years ago

@danny-smit requested a review from saltstack-formulas/ssf as a code owner 9 hours ago

Did I cause this? If so, it wasn't intentional.

By the way, it will fail anyway due the bintray downtime, but it seems that fixing my commit identity doesn't automatically trigger the Actions yet.

myii commented 3 years ago

@danny-smit requested a review from saltstack-formulas/ssf as a code owner 9 hours ago

Did I cause this? If so, it wasn't intentional.

Don't worry, this is by design due to the CODEOWNERS feature. If you touch any files owned by a particular code owner, they are requested for a review:

https://github.com/saltstack-formulas/docker-formula/blob/e44383834a42a9f7fed0910b68efe48b6b45f509/CODEOWNERS#L45

By the way, it will fail anyway due the bintray downtime, but it seems that fixing my commit identity doesn't automatically trigger the Actions yet.

That's because even your corrected commit identity hasn't had a commit merged in this formula yet, so you're still a First-time contributor. This is more restrictive than I first feared, it really shouldn't need more than one manual intervention per PR -- I'm not a fan of this decision by GitHub. There are a couple of ways around it, I've opted for the second approach:

  1. We could merge in a (no-op) commit to this repo using your identity and then your First-time contributor status should go away -- this is something that any of the maintainers could probably do on your behalf (I'm thinking of a potential strategy for our other repos as well, here).
  2. We could simply invite you as a contributor to this repo -- once you accept the invite, you shouldn't face this issue anymore.

I'm pretty sure we've "fixed it" this time!

myii commented 3 years ago

@danny-smit Whenver you see a review for saltstack-formulas/ssf selected, please leave it, otherwise I won't remember to update the ssf-formula! That formula manages the relevant files (such as kitchen.yml), so I don't want it to revert your changes later if I don't update it. Thanks.

danny-smit commented 3 years ago

@myii Sorry, I didn't mean to remove it (I don't recall I did), I only tried to request a re-review of @noelmcloughlin after my new commits.

myii commented 3 years ago

@myii Sorry, I didn't mean to remove it (I don't recall I did), I only tried to request a re-review of @noelmcloughlin after my new commits.

@danny-smit Ah, that means GitHub has another glitch to be aware of. No problem, I've bookmarked this PR anyway, even if the review request is lost again. Thanks for getting back to me about that, I can be more proactive next time.

danny-smit commented 3 years ago

The tests are now permanently failing due to the shutdown of bintray (#281). Any suggestions how we could proceed with this? I'm willing to help with #281 if there is an idea for a good solution.

myii commented 3 years ago

The tests are now permanently failing due to the shutdown of bintray (#281). Any suggestions how we could proceed with this? I'm willing to help with #281 if there is an idea for a good solution.

@noelmcloughlin Any suggestions about how this should be resolved? Can/should we disable the states in the meantime?

noelmcloughlin commented 3 years ago

Sorry for delayed response. I suggest merging this and fixing #281 in separate PR. The bintray issue is not caused by this PR.

noelmcloughlin commented 3 years ago

Regarding bintray see also https://github.com/saltstack-formulas/docker-formula/issues/281#issuecomment-835800409

danny-smit commented 3 years ago

@noelmcloughlin Thanks. Do you mind having another look at the changes I made? Since your previous approval I've added several changes with small fixes and updates to get it covered by the tests.

noelmcloughlin commented 3 years ago

@danny-smit Is it possible to fix tests?

>>>>>> Message: 3 actions failed.
>>>>>>     Converge failed on instance <archive-ubuntu-2004-master-py3>.  Please see .kitchen/logs/archive-ubuntu-2004-master-py3.log for more details
>>>>>>     Converge failed on instance <package-ubuntu-2004-master-py3>.  Please see .kitchen/logs/package-ubuntu-2004-master-py3.log for more details
>>>>>>     Converge failed on instance <repo-ubuntu-2004-master-py3>.  Please see .kitchen/logs/repo-ubuntu-2004-master-py3.log for more details
danny-smit commented 3 years ago

@noelmcloughlin Thanks. Those failing tests are strange, the tests are also failing without my changes. I think they are already solved by this new pull request: https://github.com/saltstack-formulas/docker-formula/pull/288

myii commented 3 years ago

@noelmcloughlin Thanks. Those failing tests are strange, the tests are also failing without my changes. I think they are already solved by this new pull request: #288

@danny-smit I've just hit Approve and run on #288 so we'll see the result.

danny-smit commented 3 years ago

I'm not sure what else to fix, the other failures seem to be a result of accessing bintray.com to obtain docker-compose. I can create a new pull requests which uses github to pull the archives as proposed in #281, which is fairly easy if we use a specific docker-compose version for now. If we then rebase our branches, it gives a much better picture of the state of the tests.

My only concern in doing that is how to deal with the use of the 'latest' version, as I explained in #281.

noelmcloughlin commented 3 years ago

281 is fixed by #290 and CI is passing. Can this be merged @danny-smit

danny-smit commented 3 years ago

@noelmcloughlin Yes, this is ready for merge.

saltstack-formulas-travis commented 3 years ago

:tada: This PR is included in version 2.0.5 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: