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

Ensure recover from bad tracker IDs #492

Merged
merged 6 commits into from Oct 17, 2023
Merged

Ensure recover from bad tracker IDs #492

merged 6 commits into from Oct 17, 2023

Conversation

TheDuckCow
Copy link
Member

This PR implements a few things:

  • Detect if a "bad install id" is found in the addon folder. If so, fetch a new ID or recover from the parent folder's ID (if not a bad id itself)
  • General tests for the tracker.py file to ensure it's working as intended for the top-levle public methods
  • Test to validate this new recovery mechanism works exactly as intended
  • A critical test to avoid ever shipping a new build with a prepackaged json id, which was how this whole problem started.

Closes #491

All tests pass:

-------------------------------------------------------------------------------
bversion   	ran_tests	ran	skips	failed	errors
-------------------------------------------------------------------------------
(3.6.2)   	all_tests	64	2	0	No errors
(4.0.0)   	all_tests	64	2	0	No errors
(3.5.1)   	all_tests	64	2	0	No errors
(3.4.0)   	all_tests	64	2	0	No errors
(3.3.1)   	all_tests	64	2	0	No errors
(3.2.1)   	all_tests	64	2	0	No errors
(3.1.0)   	all_tests	64	2	0	No errors
(3.0.0)   	all_tests	64	2	0	No errors
(2.93.0)   	all_tests	63	3	0	No errors
(2.90.1)   	all_tests	63	3	0	No errors
(2.80.75)   	all_tests	62	4	0	No errors
tests took 219s to run

@TheDuckCow TheDuckCow linked an issue Oct 7, 2023 that may be closed by this pull request
@TheDuckCow TheDuckCow requested a review from a team October 7, 2023 22:23
Copy link
Collaborator

@StandingPadAnimations StandingPadAnimations left a comment

Choose a reason for hiding this comment

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

Since no one else seems to want to take charge of reviewing this, I'll just add my comments.

Seems good to me, albeit with some minor nitpicks I have in terms of style, but nothing seems off in the actual logic itself.

I won't approve or reject this PR though. I'll leave that to someone else (looking at y'all @zNightlord and @MajesticalDiscomfort)

Comment on lines +282 to +288
self._tracker_idbackup = os.path.join(
os.path.dirname(__file__),
os.pardir,
self._addon + "_trackerid.json")
self._tracker_json = os.path.join(
os.path.dirname(__file__),
self._addon + "_tracker.json")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'll be worthwhile to see how this would look with pathlib, since pathlib tends to be clearer

Copy link
Member Author

Choose a reason for hiding this comment

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

idk I'm still a bit torn on pathlib always being better. It means we're more likely to stick with long lines, whereas right now I sort of like the multi line input arguments as clearly each segment of the path.

is this how you'd write that first line? (noting this goes over 80 charcters)

		self._tracker_idbackup = Path(
			os.path.dirname(__file__) / os.pardir / self._addon + "_trackerid.json")

In retrospect, the lines where we are using Pathlib are the ones most exceeding line lengths now in files like conf.py (even if we push to a newline after the ( in Path(, still exceeds line length for the definition of self.json_path_update)

I'll probably stick with this and merge this in the meantime, but feel free to comment after merging if I'm overlooking something.

MCprep_addon/tracking.py Outdated Show resolved Hide resolved
MCprep_addon/tracking.py Outdated Show resolved Hide resolved
@TheDuckCow
Copy link
Member Author

Changes pushed, going to merge shortly.

-------------------------------------------------------------------------------
bversion   	ran_tests	ran	skips	failed	errors
-------------------------------------------------------------------------------
(3.6.2)   	all_tests	64	2	0	No errors
tests took 17s to run

@TheDuckCow TheDuckCow merged commit 06d9370 into dev Oct 17, 2023
@TheDuckCow TheDuckCow deleted the safer-build-noid branch October 17, 2023 05:54
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.

Fetch new install ID if bad id found
2 participants