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

Add field configuration loading #10

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

eggmaster
Copy link
Collaborator

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.

Copy link
Member

@fuzzball81 fuzzball81 left a 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.

README.md Outdated Show resolved Hide resolved
if _json_generator is not None:
for field_data in _json_generator.get_jira_fields_json():
fields.append(JiraField(**field_data))
return fields
Copy link
Member

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.

Copy link

@lhh lhh left a 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

Copy link
Member

@jguiditta jguiditta left a 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.

tests/unit/test_config.py Outdated Show resolved Hide resolved
@eggmaster
Copy link
Collaborator Author

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.

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.
@fuzzball81
Copy link
Member

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.

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.

Feel free to use toolchest here, it makes yaml handling really easy.

@fuzzball81 fuzzball81 merged commit 33fb6d9 into release-depot:main Sep 20, 2023
4 checks passed
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.

4 participants