fix(clean): fix syntax error with use_upstream repo #285
Conversation
|
@danny-smit This comment isn't directly related to the PR itself but your status as a 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. |
@danny-smit With respect to this, InSpec controls can be run for specific platforms only, using |
|
@myii Yes, I just checked and you are correct. The e-mail addresses didn't match.
Thanks, I'll have a look at that. |
|
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. |
@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 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. |
|
@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 #281) |
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. |
Don't worry, this is by design due to the Line 45 in e443838
That's because even your corrected commit identity hasn't had a commit merged in this formula yet, so you're still a
I'm pretty sure we've "fixed it" this time! |
|
@danny-smit Whenver you see a review for |
|
@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
force-pushed
the
fix-clean-with-use-upstream-repo
branch
2 times, most recently
from
3c9c70b
to
1cee9ca
Compare
@noelmcloughlin Any suggestions about how this should be resolved? Can/should we disable the states in the meantime? |
|
Sorry for delayed response. I suggest merging this and fixing #281 in separate PR. The bintray issue is not caused by this PR. |
|
Regarding bintray see also #281 (comment) |
|
@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. |
|
@danny-smit Is it possible to fix tests? |
|
@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 |
|
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. |
With use_upstream: repo, several errors occur: - An invalid state id is used to require the repo state - An incorrect path to include the repo state is used - A dictionary with conifguration is incorrectly passed to the onlyif statement, which is already checked by the if-statement around it.
The clean failed due to a mismatch of the name of the repo that was installed.
A dictionary is passed to the salt 'onlyif' condition. This is invalid syntax and the condition is already checked by in the jinja around it.
danny-smit
force-pushed
the
fix-clean-with-use-upstream-repo
branch
from
76d926a
to
a7f382f
Compare
|
#281 is fixed by #290 and CI is passing. Can this be merged @danny-smit |
|
@noelmcloughlin Yes, this is ready for merge. |
|
🎉 This PR is included in version 2.0.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
With use_upstream: repo, several errors occur:
which is already checked by the if-statement around it.
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[build]Changes related to the build system[chore]Changes to the build process or auxiliary tools and libraries such as documentation generation[ci]Changes to the continuous integration configuration[feat]A new feature[fix]A bug fix[perf]A code change that improves performance[refactor]A code change that neither fixes a bug nor adds a feature[revert]A change used to revert a previous commit[style]Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)Secondary type
[docs]Documentation changes[test]Adding missing or correcting existing testsDoes 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: repois used:which is already checked by the if-statement around it.
Pillar / config required to test the proposed changes
Debug log showing how the proposed changes work
Documentation checklist
README(e.g.Available states).pillar.example.Testing checklist
state_top).Additional context
I attempted to add tests for the
use_upstream: repocase, 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.