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

force_cron_run: Update soft_fail reference #3947

Merged
merged 1 commit into from
Dec 2, 2017

Conversation

okurz
Copy link
Member

@okurz okurz commented Nov 24, 2017

No description provided.

@okurz
Copy link
Member Author

okurz commented Nov 24, 2017

@coolo

@@ -9,7 +9,7 @@

# Summary: Avoid suprises later and run the cron jobs explicitly
# Maintainer: Stephan Kulow <coolo@suse.de>
# Tags: bsc#1017461
# Tags: bsc#1017461, bsc#1063638
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you replace bugref why you not replace it here but just add ? old version still relevant somehow ?

and out of review question - what this "Tags" mean ? Is it just some convention or there is some mechanism in perl to handle this ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. The old reference is a very important bug so I would like to keep a reverence to it. It's only a convention so far. I don't know of any script that is extracting it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep a reverence? That's very important - we should respekt our bugs way more :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked - your "very important" bug has state "VERIFIED FIXED" . If you insists to have it here anyway , please move it into separate line with clear explanation why it so important and how it's related to this module. Comments suppose to help people to understand code better and not just leave some side notes which can be understand by few people

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIXED bugs are prime reason for regression tests, what is your point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that I don't understand what this "Tags" list suppose to tell me. This module had two softfailures : bsc#1064834 , bsc#1017461 and only second one was mentioned in "Tags" . Now bsc#1017461 was removed from code and replaced with bsc#1063638 . But "Tags" suppose to keep bsc#1017461 add bsc#1063638 and keep ignoring bsc#1064834. Bug bsc#1017461 is about BTRFS but nothing in test name or comments in tests explain me how this bug related to this test. I am sure that @coolo , @okurz have a perfect picture in there head about this but I don't have it and I am in big doubt that everyone have it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1017461 is the whole reason this test exists at all - 1063638 is the reason the test still exists. Clear now? The tags: comment is basically that: a comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah thanks , now it's clear for me but still unclear for person who will read this comment without reading all this discussion :) . So it will be nice if information from your message will migrate to comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bug is VERIFIED FIXED because I did that. A description for "Tags" could end up in the README file of the repo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coolo Yes, I guess for bsc#1017461 "reverence" is really the right word :D

@okurz
Copy link
Member Author

okurz commented Dec 2, 2017

I assume your comments mean "approval"

@okurz okurz merged commit ba8da46 into os-autoinst:master Dec 2, 2017
@okurz okurz deleted the enhance/soft_fail_force_cron_run branch December 2, 2017 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants