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

Change Bugzilla collector and Flaw model to allow multi-components in bz_summary #426

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

costaconrado
Copy link
Contributor

@costaconrado costaconrado commented Jan 24, 2024

This PR changes the way Bugzilla collector parse summary.

This PR changes flaw model so component can support multiple components.
This PR also changes the parser of bugzilla collector to bz_summary be parsed as a list instead of a single component.
This PR introduces a major change in API (component became components) and the filter for flaws changed in order to support it.
This PR also adds a property called component_str for checks that uses component as a condition.

Closes OSIDB-1420.

@costaconrado
Copy link
Contributor Author

To reviewer: Sorry for the single commit PR with multiple changes, the migration should be the 1st and it makes the API fail the tests making it unable to be in a different commit.

@costaconrado costaconrado requested a review from a team January 24, 2024 13:56
Copy link
Contributor

@Elkasitu Elkasitu left a comment

Choose a reason for hiding this comment

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

This change is not backwards compatible and contains non-safe migrations, the easiest solution at this point in time would probably be:

  • Keep component field as-is
  • Deprecate component field in the API
  • Create new field components, prefill with existing component field
  • Expose components field in API

Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

The new functionality looks OK to me but I agree with @Elkasitu that we should for now keep the old component field for the backward compatibility. It should be deprecated and maybe just return the first item from components list.

@costaconrado costaconrado force-pushed the fix-summary-parser branch 4 times, most recently from 82740c0 to bf2d354 Compare January 30, 2024 15:57
@costaconrado
Copy link
Contributor Author

Makes sense, I've deprecated the component field and added the components. @osoukup do you think it would be better to return the 1st value of components or split the value by colon and do a contains? (e.g. if user pass golang: archive/zip I return all flaws that contains both golang and archive/zip)

@costaconrado costaconrado force-pushed the fix-summary-parser branch 2 times, most recently from ba3ca98 to 8cb305e Compare January 30, 2024 16:17
@osoukup
Copy link
Contributor

osoukup commented Jan 31, 2024

Makes sense, I've deprecated the component field and added the components. @osoukup do you think it would be better to return the 1st value of components or split the value by colon and do a contains? (e.g. if user pass golang: archive/zip I return all flaws that contains both golang and archive/zip)

@costaconrado I am not sure I understand the question 👼 I mean that the value of the old attribute component should simply be the first item in the new components list attribute. It was just mean for the backwards compatibility so there is some meaningful value until we deprecate it full. It is probably not very important which item from components would be returned as the component - first/last/random/whatever. Just some meaningful value. It will be temporary anyway.

Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

I propose slight changes in how the old component is treated - it is rather a single component even though there are multiple of them in the original Bugzilla summary and I think it should be still consider so before we remove it.

osidb/filters.py Outdated
@@ -268,6 +271,16 @@ def is_major_incident_filter(self, queryset, name, value):
]
)

def component_filter(self, queryset, name, value):
"""
Based on the `value`, returns all Flaws which contains all components listed separed by colon.
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: separed to separated

osidb/filters.py Show resolved Hide resolved
osidb/models.py Outdated Show resolved Hide resolved
osidb/serializer.py Outdated Show resolved Hide resolved
@costaconrado costaconrado force-pushed the fix-summary-parser branch 6 times, most recently from a605f8c to 4290c58 Compare February 19, 2024 15:33
@costaconrado costaconrado force-pushed the fix-summary-parser branch 2 times, most recently from 81ef217 to 98bbcc7 Compare March 12, 2024 13:27
@costaconrado
Copy link
Contributor Author

I have changed the behavior so component returns the last value of components returning the same value it would return without this PR. Also if user pass something like golang: net/http to the old field it will be not parsed, just accepted as a single component.
I have also removed the field component from tests that uses apps.workflows.checks, the previous solution created a component_str property in flaw model but as discussed before it currently not is being used anywhere but in the test.

Please take a second look in this PR when you have some time @osoukup. Thanks!

Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

LGTM

osidb/filters.py Outdated Show resolved Hide resolved
osidb/tests/endpoints/test_endpoints.py Show resolved Hide resolved
@costaconrado costaconrado added this pull request to the merge queue Mar 26, 2024
Merged via the queue into master with commit 0fa2122 Mar 26, 2024
10 of 11 checks passed
@costaconrado costaconrado deleted the fix-summary-parser branch March 26, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants