-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: ieee-rawbib fetcher #24
Conversation
915d2eb
to
13aa735
Compare
@ronaldtse (cc @andrew2net) As can be seen in the GHA logs for this PR, the token currently used in the |
dca243d
to
e086e8b
Compare
It seems like this secret has been removed from this project. I added a "test" step to check if @CAMOBAP we might not need to use username:password (as I was suggesting here), but simply provide this secret and clone the repository using the canonical Alex, could you point me at someone who has access to this repository settings? |
@stefanomunarini IDK, @ronaldtse maybe you can suggest a right person |
Hi @kesara ,
@kesara as outlined above, this repository is missing a secret, AKA |
This is set. It's a personal access token for a bot account that has access to a private repository. Earlier versions of the GHA used this to access the ieee-rawbib repository. |
The secrets are not passed to workflows that are triggered by a pull request from a fork. So the PR tests will continue to fail. |
I see, this was the issue then! Would it be possible to enable passing secrets to workflows that are triggered by a PR? This is only to test the implementation before merging the PR, it could be disabled right after testing is completed. |
@stefanomunarini I don't think GitHub supports passing secrets to forks due to security concerns, for example, if a public user can create a PR, that means they can send the secret elsewhere eg via a remote call. |
Right, makes total sense. I wonder if the only way to test this implementation is merging it (in this case, can you @CAMOBAP review this PR, please?)? Is there no other way otherwise? |
bace090
to
e086e8b
Compare
I don't think there's a common alternative. @CAMOBAP can you please help check, thanks! |
When this PR is ready, I'm happy to merge and I can revert the change if it doesn't work. |
@stefanomunarini if PR is ready for review please press "Ready for review" because usually "draft" PRs mean that the work is in progress and it doesn't ready for review BTW there is no objections from my side, once workflow actually works |
You're right, that was an oversight from my side. Sorry about that @kesara feel free to merge the PR, thanks |
@stefanomunarini should we rerun the CI because last message was "fatal: Authentication failed for 'https://github.com/ietf-ribose/ieee-rawbib.git/'" |
@CAMOBAP This issue has been discussed above, in particular Ronal mentioned that:
thus the authentication failed message. Hence, we need to merge this PR in order to be able to test it. Kesara offered to roll it back immediately in the event of a CI failure. |
GHA is running here: https://github.com/ietf-tools/relaton-data-ieee/actions/runs/6754213519/job/18361482188 |
And success. Can someone confirm the errors in |
@CAMOBAP @andrew2net can you have a look at the comment above? Is this expected or is there anything that needs to be fixed? |
@stefanomunarini crawler.rb looks right to me (probaby we can remove duration calculation logic out of it because @andrew2net to me it looks like we have some issue in
|
@CAMOBAP The last files from 2023 caused the error because address country not found in them. But I'm unable to get and explore these files from the ietf-tools/ieee-rawbib repo. Is there any way to get one of the files using GHA? |
@andrew2net I think the right way will be to ask @stefanomunarini @kesara to provide required files directly (because they are in the private repo) @stefanomunarini @kesara is it possible? |
I just emailed the requested file. |
@kesara could you please help provide us access to the ieee-rawbib repository so that we can address the newly introduced parsing errors from the new files? Thanks! |
The file contains an address without country: ...
<address>
<city>New York</city>
</address>
... @kesara @ronaldtse For the NY city we can add the USA country. All the other addresses also have US cities. It seems we can use USA as a default country, right? |
No description provided.