-
Notifications
You must be signed in to change notification settings - Fork 16
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
refactor riot-os.org data update #17
Conversation
@aabadie could you give this a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's still room for improvement and the already commit data files (that are now generated on the fly) could also be removed.
RIOT_BOARDS_FILE = $(DATA_DIR)/riot_boards.yml | ||
RIOT_STATS_FILE = $(DATA_DIR)/riot_stats.yml | ||
RIOT_CONTRIBUTORS_FILE = $(DATA_DIR)/contributors.json | ||
RIOT_DRIVERS_FILE = $(DATA_DIR)/riot_drivers.csv | ||
RIOT_DRIVERS_CATS_FILE = $(DATA_DIR)/riot_drivers_cats.csv | ||
RIOT_CPUS_FILE = $(DATA_DIR)/riot_cpus.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it means that all these yml files and contributors.json committed in the repo could be git rm'ed and .gitignore'd ? (since they are regenerated automatically)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we were thinking with Leandro to leave some as a stub (so one doesn't need to call the update target). But this would be indeed possible.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if someone generates the website locally to get a local preview, all these files will end up modified (in git), so this will be annoying.
Better remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But one could build the website locally without calling make update_riot_data
, then the files would not be updated and the stub values would be used instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we can add a .gitignore
and then those files would still be there, but won't be tracked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it introduces additional (unnecessary) steps
As I said, it doesn't introduce additional steps for the user, since it's automatically handled by make build
the first time.
we want to provide a 'minimal' running version that might include outdated data but shows the content elements.
Then why all these stub files are taken from the generator directly. If only the content elements are important (whatever this means), you could simply have dummy data (like fake data), that wouldn't necessarily come from the RIOT website. I mean there's no need to commit a 400 lines files containing all boards supported in February 2021 or a 2k file containing all contributors (and also containing unnecessary information) . Let's also mention this one line json file of 20k columns for the drivers! And this is without mentioning the mixed formats (some in json, some in yml and, with this PR, some in csv). This is just cluttering the repository (repeating myself).
Again, since they are generated, they shouldn't be committed in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like having stub files with subsets of the lists is a good tradeoff between having empty pages when not updating, and cluttering the repo with huge files. But in this case make build
would not have to trigger an update, right? Should we proceed with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why all these stub files are taken from the generator directly. If only the content elements are important (whatever this means), you could simply have dummy data (like fake data), that wouldn't necessarily come from the RIOT website.
but it doesn't harm either. taking a snapshot and use this as dummy data is completely fine. ... anyhow, as @leandrolanzieri suggested, he will include a reduced set of data for each generated file, to spare some space.
i think we can proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty clear that generated data shouldn't be committed.
Note that the only reason why we were doing this was for generating dummy files. Sorry if it was not clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed, I reduced the stub files. I chose some numbers that allow to test search functionalities and responsiveness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
riot_drivers_cats.csv does not include any stub data.
Fixed with the last push. I added proper stub files and a make target |
it seems all remarks are addressed except the reduced amount of data in the stub files, right? |
Still needs squashing and AFAIK #17 (comment) was not yet fulfilled. |
Co-authored-by: José Alamos <jose.alamos@haw-hamburg.de>
6565df9
to
3da1391
Compare
rebased |
@aabadie are you ok with this PR? |
i just gave it a nother test and it looks good! |
last call for objections. @aabadie? we want to make progress because this PR is also a requirement for decent previews of other PRs. |
Contribution Description
This PR adds scripts for updating RIOT data (contributors, cpus, boards, etc).
These actions can be invoked using
make
targets (seeREADME.md
).This PR simplifies GH Actions deployment.
Also fixes #25.