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

check_tags: warn when issue references are removed (plus other fixes) #679

Merged
merged 5 commits into from Feb 17, 2017

Conversation

@jberry-suse
Copy link
Contributor

jberry-suse commented Feb 10, 2017

Requested in #677.

  • handle failed to load issue diff for existing package.
  • correct backwards diff.
  • use len() instead of list() since only count is of interest.
  • warn when issue references are removed.
  • grammatical corrections.

Interestingly enough the diff would appear to have been performed backwards. Since only the existence of issues was checked this worked fine, but given the request was about warning when removed it seemed proper to flip rather than look for added when meaning deleted.

For example, sr#455816 which is clearly adding one bug reference showed it as being deleted:

<sourcediff key="c9b42dc9c3a1f9435342cbe12cad4d3d">
  <old package="python3-requests.6329" project="openSUSE:Leap:42.2:Update" rev="baa02f86911ca86af78b4f0db1b7b882" srcmd5="baa02f86911ca86af78b4f0db1b7b882" />
  <new package="python3-requests" project="openSUSE:Leap:42.3" rev="4" srcmd5="1809a2399024102a6b19ea1ea83e79f2" />
  <files />
  <issues>
    <issue label="boo#912903" name="912903" state="deleted" tracker="bnc" url="https://bugzilla.opensuse.org/show_bug.cgi?id=912903" />
  </issues>
</sourcediff>

Whereas after reversing the diff:

<sourcediff key="148e0ab7e2d155c8933bd5d73d78691d">
  <old package="python3-requests" project="openSUSE:Leap:42.3" rev="4" srcmd5="1809a2399024102a6b19ea1ea83e79f2" />
  <new package="python3-requests.6329" project="openSUSE:Leap:42.2:Update" rev="baa02f86911ca86af78b4f0db1b7b882" srcmd5="baa02f86911ca86af78b4f0db1b7b882" />
  <files />
  <issues>
    <issue label="boo#912903" name="912903" state="added" tracker="bnc" url="https://bugzilla.opensuse.org/show_bug.cgi?id=912903" />
  </issues>
</sourcediff>
@jberry-suse jberry-suse requested a review from lnussel Feb 10, 2017
@jberry-suse jberry-suse force-pushed the jberry-suse:tags-delete-warn branch from 7cd81e5 to d587061 Feb 10, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 10, 2017

Coverage Status

Coverage remained the same at 45.346% when pulling d587061 on jberry-suse:tags-delete-warn into 4c5ebff on openSUSE:master.

self.review_messages['accepted'] = 'New package'
return True

self.review_messages['accepted'] = 'Warning: failed to load package diff'
return True

This comment has been minimized.

Copy link
@lnussel

lnussel Feb 14, 2017

Member

hmm, this would accept requests that cannot be diffed. I guess None would emulate the old behavior when the bot crashed.

This comment has been minimized.

Copy link
@jberry-suse

jberry-suse Feb 14, 2017

Author Contributor

Yes, so previously when failed to load diff it would only return True when is_new, which actually meant it would crash entirely on xml = ET.parse(f) and fail to mark the review either way. This new code will accept in both cases, but include the warning (which was not present before).

This comment has been minimized.

Copy link
@jberry-suse

jberry-suse Feb 14, 2017

Author Contributor

I re-read what you wrote. Did you want to change this to return None is the request is not updated? I imagine it may remain open forever if the diff issue is not transient. In the case of new we seem to agree it needs to remain True.

This comment has been minimized.

Copy link
@lnussel

lnussel Feb 15, 2017

Member

yes, if we return None the review may stay open forever until someone overrides it or fixes the cause. I fear that if we return True right away the bot will approve requests on transient failures. Sometimes OBS is overloaded to create diffs and errors out IIRC. At a later time it may work again.

This comment has been minimized.

Copy link
@jberry-suse

jberry-suse Feb 15, 2017

Author Contributor

Change to return None.

@jberry-suse jberry-suse force-pushed the jberry-suse:tags-delete-warn branch from d587061 to 8e842e5 Feb 15, 2017
@jberry-suse jberry-suse changed the title check_tags: warn when issue references are removed (plus other fixed) check_tags: warn when issue references are removed (plus other fixes) Feb 15, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 15, 2017

Coverage Status

Coverage remained the same at 45.475% when pulling 8e842e5 on jberry-suse:tags-delete-warn into b3129e9 on openSUSE:master.

@lnussel lnussel merged commit b955c11 into openSUSE:master Feb 17, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jberry-suse jberry-suse deleted the jberry-suse:tags-delete-warn branch Mar 8, 2017
jberry-suse added a commit to jberry-suse/openSUSE-release-tools that referenced this pull request May 15, 2019
See openSUSE#679, where it was originally added and provides example output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.