-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add field configuration loading #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Just a few suggestions.
if _json_generator is not None: | ||
for field_data in _json_generator.get_jira_fields_json(): | ||
fields.append(JiraField(**field_data)) | ||
return fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): This feels like it could be a class with a class method to register the generator and then normal method to get the fields. The register method could then take a type and when a get_fields is called the correct list of fields is returned. https://realpython.com/factory-method-python/ has some good examples. This is something that can be done in a follow up patch to keep the churn and size of this one down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an excellent start.
Notes:
- JIRA inherently returns almost all fields for a given issue whenever you call get on it (it's all in the JSON). Issue links, external trackers, and attachments are a secondary API call (which could be lazy loaded) - BUT
- Expanding on what eggs points out, JIRA field names are often not human-readable - that is, when you get an issue JSON blob back from a JIRA server, it includes a lot of
customfield_12345678978432
things, but no human-readable description for what that means. It can be discerned by checking that server's/rest/api/latest/field
API endpoint, but the human-readable information is not present in the JSON data for a given issue, making resolving each field dynamically is inefficient unless we either cache this information or simply provide it as a configuration item to the broker, which appears to be what we've done here. - By contrast, bugzilla does the opposite of JIRA - it doesn't give you anything unless you explicitly ask, and then it really slows down if you lazy-load fields one at a time, so we may want the person instantiating the broker object to provide the list of fields loaded ahead of time. See: https://github.com/python-bugzilla/python-bugzilla/blob/main/bugzilla/bug.py#L73
f46e077
to
0e00f8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think this looks good for a first round of implementation. I like @fuzzball81 's suggestion about adjusting the encapsulation/design a bit, but I think it is fine to address that in a later patch. Just curious, what was the reason for specifying the config supplied by the user as json vs yaml, like we do in other projects? It is fine, I just personally find yaml much easier to edit and work with.
I have one requested change in a test, but will approve after that.
Mostly just unfamiliarity with yaml. I agree it'd be more user-friendly for the config file stuff. I'll create a task to switch over to using yaml in the future. |
Because issue field configuration in JIRA is highly dynamic and customizable, Bugjira needs to be able to retrieve field configuration data from an external source. This patch adds support for that operation, and also defines a very minimal set of field configuration data. The classes in bugjira/field.py will be heavily extended. Once we can make Bugjira aware of the available fields in the associated JIRA and Bugzilla backends, we will be able to add support for querying and updating those field attributes in BugjiraIssue objects. The module that loads the field configuration data is designed as a plugin. The plugin included in the source code loads json from a local file when the Bugjira instance is created. Another approach would be to load field configuration by querying a JIRA instance directly, possibly dynamically updating the field info periodically or even each time a BugjiraIssue's fields are accessed. These alternate field info access approaches could be coded into an external library that implements the plugin interface, thus allowing us to decouple the public Bugjira code from any user's particular JIRA instance configuration.
0e00f8c
to
c5ccf1d
Compare
Feel free to use toolchest here, it makes yaml handling really easy. |
Because issue field configuration in JIRA is highly dynamic and customizable, Bugjira needs to be able to retrieve field configuration data from an external source. This patch adds support for that operation, and also defines a very minimal set of field configuration data. The classes in bugjira/field.py will be heavily extended.
Once we can make Bugjira aware of the available fields in the associated JIRA and Bugzilla backends, we will be able to add support for querying and updating those field attributes in BugjiraIssue objects.
The module that loads the field configuration data is designed as a plugin. The plugin included in the source code loads json from a local file when the Bugjira instance is created. Another approach would be to load field configuration by querying a JIRA instance directly, possibly dynamically updating the field info periodically or even each time a BugjiraIssue's fields are accessed. These alternate field info access approaches could be coded into an external library that implements the plugin interface, thus allowing us to decouple the public Bugjira code from any user's particular JIRA instance configuration.