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

Rename _schedule.conf on YAML parse error #58179

Merged
merged 50 commits into from Sep 30, 2020

Conversation

marbx
Copy link
Contributor

@marbx marbx commented Aug 12, 2020

What issues does this PR fix?

Merge requirements satisfied?

@marbx marbx requested a review from a team as a code owner August 12, 2020 18:14
@ghost ghost requested review from DmitryKuzmenko and removed request for a team August 12, 2020 18:15
@DmitryKuzmenko
Copy link
Contributor

@markuskramerIgitt Personally I don't like the idea to remove any data that is possibly edited by user. Maybe it's better to just skip and keep it?

@twangboy twangboy force-pushed the delete_schedule_conf_on_yaml_parse_error branch from 9585e71 to 07c18c3 Compare August 13, 2020 00:08
@marbx
Copy link
Contributor Author

marbx commented Aug 13, 2020

@DmitryKuzmenko you are right, I took over your suggestion.
This enables me to analyze how often minions invalidate their configurations.
In the long run, someone need to understand why the salt-minion writes NUL into _schedule.conf

@twangboy thank you for the test code!
I adapted your code (rename instead or remove).

DmitryKuzmenko
DmitryKuzmenko previously approved these changes Aug 13, 2020
@DmitryKuzmenko
Copy link
Contributor

DmitryKuzmenko commented Aug 13, 2020

@markuskramerIgitt could you please resolve lint and pre-commit issues? Also your test code should be added to tests/unit/test_config.py instead of tests/unit/config/test_config.py.

@marbx
Copy link
Contributor Author

marbx commented Aug 14, 2020

@markuskramerIgitt could you please resolve lint and pre-commit issues? Also your test code should be added to tests/unit/test_config.py instead of tests/unit/config/test_config.py.

@DmitryKuzmenko where shall I add the test?

Is this related to the error from unit.test_module_names.BadTestModuleNamesTestCase.test_module_name_source_match

The following 1 test module(s) could not be matched to a source code file:
unit.config.test_config (expected: salt\config\config.py)

How do I run the test locally?

@DmitryKuzmenko
Copy link
Contributor

@markuskramerIgitt you shall move your test code from your new file tests/unit/config/test_config.py to the existing one tests/unit/test_config.py. This is related to the test failure because the test system is validating file names match. If you are testing a code from subdir/__init__.py you shall to use test_subdir.py name.

@DmitryKuzmenko
Copy link
Contributor

@markuskramerIgitt sorry, I've missed the question about running tests.
To run tests locally you can use nox like this:

$ nox -e 'pytest-zeromq-3.8(coverage=False)' -- -v --run-slow tests/unit/test_config.py

To run pre-commit hooks you can use this command:

$ pre-commit run salt/config/__init__.py

Surely you need to install the related dependencies like pre-commit and nox

@marbx
Copy link
Contributor Author

marbx commented Aug 21, 2020

@dwoz All checks have passed. Could you please remove the has-failing-test label?

@marbx marbx changed the title Delete schedule conf on yaml parse error Rename _schedule.conf on YAML parse error Aug 24, 2020
@marbx
Copy link
Contributor Author

marbx commented Aug 29, 2020

@sagetherage you removed "merge-ready":

ci/py3/freebsd121 fails with "Caused: hudson.plugins.git.GitException: Could not checkout 4d7f55c"

I assume this is unrelated to this PR, please support.

Also, a number of unrelated ("boto") files are in the PR - I assume because I used a

git checkout --theirs FILE-WITH-MERGE-CONFLICT

does it help if I create a new PR?

@DmitryKuzmenko
Copy link
Contributor

@markuskramerIgitt please don't care about it. The Merge-ready tag is just dropped from labels because we've found it useless. We don't use it to decide merge a PR or not. The decision is based on CI checks, code review and having tests covering the change.

@twangboy twangboy self-assigned this Sep 23, 2020
@twangboy twangboy added this to the Magnesium milestone Sep 23, 2020
@twangboy twangboy added the Magnesium Mg release after Na prior to Al label Sep 23, 2020
@dwoz dwoz merged commit 1a37f5b into saltstack:master Sep 30, 2020
26 checks passed
@OrangeDog
Copy link
Collaborator

OrangeDog commented Nov 30, 2020

This PR's commits are a big mess.
There are 16 separate "delete_schedule_conf_on_yaml_parse_error", for example.
This should not have been merged in this state.

It's also caused bugs completely unrelated to _schedule.conf

@marbx
Copy link
Contributor Author

marbx commented Nov 30, 2020

@OrangeDog I agree that my commits to this PR are confusing

Next occasion I will scratch a PR branch with more than one commit and open a new one with a single commit.

The reason for renaming a broken _schedule.conf file is that 1) the minion will no longer contact or respond to the master and 2) the Windows client generates a high CPU load.

@OrangeDog
Copy link
Collaborator

a new one with a single commit

No, don't do that either. Rebase and/or amend so all the fixes go in the correct commits and force push. Don't use the same message for multiple commits.

@marbx
Copy link
Contributor Author

marbx commented Dec 19, 2020

a new one with a single commit

No, don't do that either. Rebase and/or amend so all the fixes go in the correct commits and force push.

@OrangeDog,
I try to understand what happened.

In August I made a Rebase and resolved conflicts by git checkout --theirs FILE-WITH-MERGE-CONFLICT
As far as I can remember, I only did that for "my files", and as far as I understand rebasing, this is an inevitable step in rebasing.
So far this caused no problem.

The boto files where introduced into this PR by this rebase.
As far as I understand, this is the purpose of a rebase: to add changes to the PR that happened on master after one created the branch.

So far still no problem: "unrelated files" are regular part of a rebased PR.

Where I think the problem arises, is that "unrelated files" should or even must not be changed.
In this PR, the changes where fatal.

Is that correct?

@OrangeDog
Copy link
Collaborator

@markuskramerIgitt If those changes were on master then they wouldn't show up in this PR. You must have done something else, either pulling them from another branch, or reverting fixes that had already been merged. That is separate from the fact that your own changes include 16 identically-named commits.

The blame does not all fall on you. The reviewers should also have noticed and nobody should have merged this.

@marbx marbx deleted the delete_schedule_conf_on_yaml_parse_error branch July 1, 2021 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants