Skip to content

sort glob() output for consistent results#16

Merged
binary1230 merged 1 commit intomasterfrom
sort_glob
Jun 15, 2016
Merged

sort glob() output for consistent results#16
binary1230 merged 1 commit intomasterfrom
sort_glob

Conversation

@binary1230
Copy link
Copy Markdown
Contributor

@binary1230 binary1230 commented May 28, 2016

Since glob() returns an ordering that is os-dependent, we need to sort here in order to ensure that all OS'es load the non-prioritized plugins in the same order

This change does not fix any loading issues, BUT it will ensure that if there IS a load order issue, it will happen either everywhere or nowhere. This eliminates "why is it working on my machine but not on the server".

sidenote: Originally I wanted to do this change as a precaution, however, it turns out it's necessary for my computer and magstock staging server to load things in the same order.

before fix:

non-prioritized plugin load order on magfest staging server:

['bands', 'barcode', 'magstock', 'uber_analytics']

non-prioritized plugin load order on my box:

['magstock', 'barcode', 'uber_analytics', 'bands']

after fix:

both environments now load in the same order:

['bands', 'barcode', 'magstock', 'uber_analytics']

since glob() returns an ordering that is os-dependent, we need to sort here in order to ensure
 that all OS'es load the non-prioritized plugins in the same order

 this change does not fix any loading issues, BUT it will ensure that if there's an issue on one server,
 there will be an issue everywhere (no inconsistent issues on different OSes)
@binary1230
Copy link
Copy Markdown
Contributor Author

fixes #15

@binary1230
Copy link
Copy Markdown
Contributor Author

in this example btw, magstock needs to get loaded after bands plugin because it overrides some stuff there.

the RIGHT answer is to put the band plugin in priority_plugins (a separate change that will be coming)

but, it's really confusing without this change because on my box band plugin happens to load before magstock (correct) and on staging it just happens to load after magstock (broken). so this change just makes sure either both boxes work, or they both fail. This eliminates one source of possible inconsistencies between multiple environments.

@kitsuta
Copy link
Copy Markdown
Member

kitsuta commented May 28, 2016

I've been ambivalent about whether this change was really needed but it seems you do keep running into trouble. If nothing else, this will catch config errors more reliably.

That being said, it looks like the Travis build failed?

@binary1230
Copy link
Copy Markdown
Contributor Author

Yea seems to be falling what looks like an unrelated unit test only for
python 2.7, weird
On May 28, 2016 3:47 PM, "Victoria Earl" notifications@github.com wrote:

I've been ambivalent about whether this change was really needed but it
seems you do keep running into trouble. If nothing else, this will catch
config errors more reliably.

That being said, it looks like the Travis build failed?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#16 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AFKYyEAe_BLro8XkqFVdoC8fImy1u9g-ks5qGJvfgaJpZM4IpEqN
.

@kitsuta
Copy link
Copy Markdown
Member

kitsuta commented Jun 15, 2016

@binary1230 Do you want to merge anyway?

@binary1230
Copy link
Copy Markdown
Contributor Author

hey so I just re-ran the travis build and this time it worked, it looked like some kind of transient thing. The sideboard tests open websockets and stuff, so it's possible something got janky there.

@binary1230 binary1230 merged commit 9c51322 into master Jun 15, 2016
@binary1230 binary1230 deleted the sort_glob branch June 15, 2016 10:34
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.

2 participants