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

Setup github action to sync external plugins #1

Merged
merged 44 commits into from
Jun 5, 2022
Merged

Conversation

urwrstkn8mare
Copy link
Member

@urwrstkn8mare urwrstkn8mare commented May 27, 2022

I just wrote this up after looking at various GitHub actions documentation and combining some GitHub actions. Haven't gotten the time to test it yet but I think it would be easier for @UncleGoogle to test this anyway. Please have a look and will appreciate any suggestions.

PS. Then we can use the same workflow that is used for all plugins (internally maintained or otherwise) to create releases and update current_version.json, etc. In fact, I suggest we create another custom GitHub action that abstracts some of the repetitive stuff out of said workflow.

PSS. github codespaces is pretty cool

README.md Outdated Show resolved Hide resolved
@UncleGoogle
Copy link
Contributor

@urwrstkn8mare awesome, brilliant! I'm excited 🚀

I'll test it for sure. Sadly - won't test in a next few days. Tuesday is more realistic, so if you have time - feel free to close PR in hubmle bundle fork and setup synchronizer there

BTW If I understand github action versioning correctly I can test it before merging by specyfing syncer@branch_name ?

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@urwrstkn8mare
Copy link
Member Author

Oh I almost forgot I have my own plugin to test with, although it isn't in built-in search I hope you guys don't mind me forking it to the org to at least test.

@urwrstkn8mare
Copy link
Member Author

For whatever reason the workflow isn't running so if someone is able to check what's going on I'd greatly appreciate (see https://github.com/GOG-Nebula/galaxy-riot-integration)

@AndrewDWhite
Copy link

AndrewDWhite commented May 28, 2022

For whatever reason the workflow isn't running so if someone is able to check what's going on I'd greatly appreciate (see https://github.com/GOG-Nebula/galaxy-riot-integration)

The yml is not valid. If you would like a more detailed answer let me know and I can look at it more later when I feel better.
https://github.com/GOG-Nebula/galaxy-riot-integration/actions/runs/2399870420
https://github.com/GOG-Nebula/syncer/blob/8d3eacaa7733c987da341392202c8aff039f72c1/action.yml#L24

An example of how you can check this locally.

$ python -c 'import yaml, sys; print(yaml.safe_load(sys.stdin))' < action.yml Traceback (most recent call last):
File "", line 1, in
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml_init_.py", line 162, in safe_load
return load(stream, SafeLoader)
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml_init_.py", line 114, in load
return loader.get_single_data()
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\constructor.py", line 41, in get_single_data
node = self.get_single_node()
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 36, in get_single_node
document = self.compose_document()
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 55, in compose_document
node = self.compose_node(None, None)
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 84, in compose_node
node = self.compose_mapping_node(anchor)
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 133, in compose_mapping_node
item_value = self.compose_node(node, item_key)
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 84, in compose_node
node = self.compose_mapping_node(anchor)
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 133, in compose_mapping_node
item_value = self.compose_node(node, item_key)
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 82, in compose_node
node = self.compose_sequence_node(anchor)
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 111, in compose_sequence_node
node.value.append(self.compose_node(node, index))
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 84, in compose_node
node = self.compose_mapping_node(anchor)
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\composer.py", line 127, in compose_mapping_node
while not self.check_event(MappingEndEvent):
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\parser.py", line 98, in check_event
self.current_event = self.state()
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\parser.py", line 428, in parse_block_mapping_key
if self.check_token(KeyToken):
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\scanner.py", line 116, in check_token
self.fetch_more_tokens()
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\scanner.py", line 223, in fetch_more_tokens
return self.fetch_value()
File "C:\Users\andyn\AppData\Local\Programs\Python\Python37-32\lib\site-packages\yaml\scanner.py", line 579, in fetch_value
self.get_mark())
yaml.scanner.ScannerError: mapping values are not allowed here
in "", line 24, column 33

@urwrstkn8mare
Copy link
Member Author

urwrstkn8mare commented May 28, 2022

Ok so there is an issue with this. I'm not sure whats wrong but I gotta fix it later. I think the create-pull-request action doesn't work as I thought it did. If you guys have any suggestions please feel free.

@urwrstkn8mare
Copy link
Member Author

urwrstkn8mare commented May 28, 2022

Now I rewrote it as a typescript GitHub actions which theoretically is more efficient and straightforward. But now the jobs are returning this error: https://github.com/GOG-Nebula/galaxy-riot-integration/runs/6637836565?check_suite_focus=true. I don't know how to trace the error so this is quite annoying. I've setup a test branch in my personal repo and forked the master branch here. So its ready to test. Any help would be greatly appreciated (also feel free to directly commit to the master branch of the forked riot plugin as its just there for testing for now).

@urwrstkn8mare
Copy link
Member Author

urwrstkn8mare commented May 28, 2022

oooooooooooooh my god it works. see https://github.com/GOG-Nebula/galaxy-riot-integration/pull/1 (don't actually merge lol). The commits are messy but maybe its time to try out that squash and merge functionality. Also this could still use alot of polishing:

  • better logging in the action
  • update the readme
  • fix this repo's workflows (incl making them use yarn instead of npm)
  • add tests (maybe not worth it)
  • improve code readablitiy

So I'd appreciate any help with that as I shall now sleep.

EDIT: moved to readme as it is not something that needs to be done ASAP.

Copy link
Member Author

urwrstkn8mare commented Jun 3, 2022

@UncleGoogle I've got it working now, see: GOG-Nebula/galaxy-integration-humblebundle#4

I've also made it readable and added plenty of logging while I was at it. (and renamed the repo)

So personally I think it is ready to merge but please have a look and let me know your thoughts.

I guess what I am indirectly thinking about and asking is this. Is is easier, i.e. a built in function, to maintain a current hash of those files, just like the lockfile does, instead of those files if the content is meaningless to humans, auto generated, and only the presence of a difference in what is generated matters?

Sorry I'm not sure what you mean here. @AndrewDWhite could you elaborate?

@AndrewDWhite
Copy link

AndrewDWhite commented Jun 3, 2022

@UncleGoogle I've got it working now, see: GOG-Nebula/galaxy-integration-humblebundle#4

I've also made it readable and added plenty of logging while I was at it. (and renamed the repo)

So personally I think it is ready to merge but please have a look and let me know your thoughts.

I guess what I am indirectly thinking about and asking is this. Is is easier, i.e. a built in function, to maintain a current hash of those files, just like the lockfile does, instead of those files if the content is meaningless to humans, auto generated, and only the presence of a difference in what is generated matters?

Sorry I'm not sure what you mean here. @AndrewDWhite could you elaborate?

Since some of the files are simply automatically generated; instead of the file being checked into the repo we would store a file that looks like the lock files for the dependencies. (There is probably something that already exists for this since it effectively duplicates the lock files but instead of dependencies it does it for the generated files.)

Here is an example entry for dist/index.js .
"@dist/index.js@": integrity sha512-df52a38d211d06913d101056ce43640391dee11abe9a5c379aeda7167210679850512f0b99e8e7951dbd44f5ce9336fa10350d917023788fde33085a8140b634

In the process, you would hash and append all of the other generated files cryptographically secure hashes into the file.
"@dist/index.js@": integrity sha512-df52a38d211d06913d101056ce43640391dee11abe9a5c379aeda7167210679850512f0b99e8e7951dbd44f5ce9336fa10350d917023788fde33085a8140b634

"@dist/index.js.map@": integrity sha512-8ad74f602c2048130f1bc29dd18e61313541399f384a557b130fd76043a9cfebb9fbb7ed00fa120a7f6aec96bf37d9b8179925fc5f7a06619f7f8c8038f26136
And so on filling the file with the hashes for the other generated files. (Note this will have all automatically generated files' hashes and not just changes.)

In the processing what you would do is generate these files normally using the automatic process. Instead of checking the hash against the hash of the file directly stored by git, instead you can check the hash against the corresponding value in the file. The advantage here is that there is less code in the repo for anyone to review for changes, and changes would be updated in that file and still validated.

image

An example of how to get the example hash value is the following.
$ sha512sum.exe index.js df52a38d211d06913d101056ce43640391dee11abe9a5c379aeda7167210679850512f0b99e8e7951dbd44f5ce9336fa10350d917023788fde33085a8140b634 *index.js

@urwrstkn8mare
Copy link
Member Author

Oh I see. Correct me if I'm wrong but doesn't github already only check in changed files?

@urwrstkn8mare
Copy link
Member Author

Oh @AndrewDWhite I think I understand what you mean, storing the hashes of dist/ instead of the files itself. Unfortunately, for github actions, the dist/ must be checked in as that is where the files for the Github action are downloaded from on run. Normally in a JS project, dist/ isn't checked in at all.

@UncleGoogle
Copy link
Contributor

Looks like github actions API does not support typescript natievly (like browsers) so that people advice commiting dist each time: https://github.com/actions/typescript-action#publish-to-a-distribution-branch

If you want to takle a risk of not being able to review minified raw dist files, what about having that check in pipeline? Basically either generate and commit it another commit by bot or guard in CI (simpler option): fail pipeline if committed dist differs from the generated dist during the CI step.

@urwrstkn8mare
Copy link
Member Author

fail pipeline if committed dist differs from the generated dist during the CI step.

Yep there's alr a workflow doing that.

Copy link

@AndrewDWhite AndrewDWhite left a comment

Choose a reason for hiding this comment

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

Manually reviewed all but the following automatically generated files:

  • dist/index.js

  • dist/index.js.map

  • dist/licenses.txt

  • dist/sourcemap-register.js

  • yarn.lock

If the first four files end up being unmanageable with this approach we can look at adding the processing steps described in the discussion into the workflow.

@urwrstkn8mare
Copy link
Member Author

Manually reviewed all but the following automatically generated files:

  • dist/index.js
  • dist/index.js.map
  • dist/licenses.txt
  • dist/sourcemap-register.js
  • yarn.lock

If the first four files end up being unmanageable with this approach we can look at adding the processing steps described in the discussion into the workflow.

For the dist/ files its ok cause if all the checks pass that means that it is what it should be. As for the yarn.lock, I don't think other JS projects do anything to deal with it when reviewing changes.

@urwrstkn8mare urwrstkn8mare merged commit 635f170 into main Jun 5, 2022
@urwrstkn8mare urwrstkn8mare deleted the feature/initial branch June 5, 2022 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants