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 in CI to check personnel database is internally consistent #74

Open
richardreeve opened this issue May 3, 2020 · 11 comments
Open

Comments

@richardreeve
Copy link
Member

#25 is dealing with creating a database to update our mailing lists, but it requires that the data in the https://github.com/ScottishCovidResponse/personnel repo is internally consistent.

In the first instance, the critical thing is to check that each file in the projects/ subdirectory has a name = "XXX" element and [people] hash, and each [people.YYY] element of the hash corresponds to a [YYY] element in the people.toml file at the top level, and the latter has an associated email address.

This issue is to add CI to do this.

@ghost
Copy link

ghost commented May 12, 2020

I've got some tests for this issue. I still need to figure out the best way to hook them in but I wanted to mention, there is a member of the management team "ruthdXXX" that is not in the people.toml file. There is a "ruthhXXX" so it may be a typo but this person is likely not on the management mailing list currently either way.

@ghost
Copy link

ghost commented May 14, 2020

Ok, I have set up the GitHub action to run the tests when a push is made. It appears to be working and has picked up an issue. @alysbrett you may be missing from one of the project mailing lists. Hopefully what I've done is ok. If so, this issue can be closed.

@ghost
Copy link

ghost commented May 14, 2020

Specifically, the data.toml looks like the culprit.

@alysbrett
Copy link
Contributor

Thanks very much @nathan-cummings-ukaea . I fixed me in data and Ruth in management so hopefully it will pass now!

@alysbrett
Copy link
Contributor

It would be really great if the email on failure had the actual error in it (ie the pytest output in this case) but no idea if that's easy or hard. And if the pytest output has any options to be less verbose so the actual assertion error is what you see in the email without much else that would be even better!

Only if it's easily done though - works fine as is and will get used to spotting the problem quickly if it happens often I imagine

@ghost
Copy link

ghost commented May 14, 2020

Yep, the tests pass as of your last commit so that’s great. I agree the test output isn’t great. I imagine it’s fairly straightforward to improve it. I’ll take a look. I bet you can make the content of the email more useful as well. I’ll let you know when I figure out how. It would be useful to know for future reference even if it isn’t strictly necessary here.

@alysbrett
Copy link
Contributor

Do close this once you've decided whether to tweak the test output.

@richardreeve
Copy link
Member Author

It would be great if we could also test that people's org labels in the people.toml file are valid (i.e. present in orgs.toml) and that email addresses are unique - I realised a couple of days ago that I had added someone in a second time when I should have just fixed their github id...

I also wonder whether it's possible to check the membership list of the org - https://developer.github.com/v3/orgs/members - against the people in people.toml to see if we're missing any?

Finally, what happens if I add someone to the people.toml file who hasn't given me their email address for the mailing lists. There are a couple of people who haven't, and before I added them I wanted to check that it wouldn't break the mailing list code... ?

@ghost
Copy link

ghost commented May 21, 2020

Sorry Richard, I've been away for the past few days but I'm back to look at this now. I'll get those tests written. As for adding someone without an email address... I honestly am not sure what it would do. It could be that it seems fine on my end but ends up being a problem when the mailing list gets populated, which I wouldn't know about.

Ideally, we would have an email address for everyone. Is the issue that you just don't have them yet, or are some people not able to provide email addresses at all?

@richardreeve
Copy link
Member Author

No problem. I just wanted to add someone’s group and despite a bit of badgering I didn’t have all of their emails. However, someone else badgered them a bit more and they came through! If it was possible just to do something like ignore people without emails when populating the database that would be great? It would just allow me to add them temporarily in the future without worrying about destroying the mailing lists!

@ghost
Copy link

ghost commented May 22, 2020

Ok, I've changed the updatedb.py script so it simply checks "if user_data['email']" before adding a user to the database. This should work, if you put someone in with an email the CI tests will fail but the user shouldn't be added to the database. If/when you do add someone without an email, let me know and I can check the database to make sure it ran ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants