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

refact: Remove hosts #330

Merged
merged 12 commits into from
Jul 28, 2023
Merged

refact: Remove hosts #330

merged 12 commits into from
Jul 28, 2023

Conversation

n-dusan
Copy link
Contributor

@n-dusan n-dusan commented Jul 27, 2023

Description (e.g. "Related to ...", etc.)

Closes #328

Removed hosts and hosts.json logic. It was necessary to remove the host handler and omit the hosts from the pull pipeline schema. Removed hosts from documentation and from testing framework.

To test, run taf repo update https://github.com/openlawlibrary/open-law

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)

Remove hosts handler. Now, instead of iterating through each host, we
simply iterate through all updated repositories and update the
dictionary response in update handler
Do not run host handler (since it is removed).
Refactored `_check_update_status` to set update status and any error
messages based on updated repositories (instead of hosts). Add minor
type hints to function signatures

Added `_update_transient_data` which updates the transient
dictionary into it's own function. This code was earlier in a for loop.
Necessary since host handler is removed
@n-dusan n-dusan requested a review from renatav July 27, 2023 11:49
@n-dusan n-dusan self-assigned this Jul 27, 2023
@n-dusan n-dusan changed the title Remove hosts refact: Remove hosts Jul 27, 2023
Copy link
Collaborator

@renatav renatav left a comment

Choose a reason for hiding this comment

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

While I agree thwt hosts is not something that TAF necessarily needs to handle (TAF is not meant to be used for hosting purposes, so it being used to determine a host was probably an overkill), 2 questions:
Do we know what was causing the issue leading to the removal of this code? Was it not worth debugging it due to change of plans or was it difficult to debug? Was removal of host handler files not sufficient? If so, then I would definitely want to get to the bottom of it

@n-dusan
Copy link
Contributor Author

n-dusan commented Jul 28, 2023

I believe the reasoning was that rather than debugging (and getting to the bottom of the issue) we would just remove the host code, while having a reference to this PR so that we could reimplement the handler if it becomes necessary. So I think the reasoning was that it wasn't worth debugging because our thinking with stelae has been to not rely on hosts information.

@n-dusan n-dusan requested a review from renatav July 28, 2023 10:19
Copy link
Collaborator

@renatav renatav left a comment

Choose a reason for hiding this comment

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

Approving, we can revert the pr if necessary

@n-dusan n-dusan merged commit cdc2473 into master Jul 28, 2023
25 checks passed
@n-dusan n-dusan deleted the ndusan/remove-hosts branch July 28, 2023 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not handle hosts and hosts.json
2 participants