Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix: Add header to gitlab.js #2158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Jan 19, 2023

🌟 What does this PR do?

Fixes error reported by CI:

reverse-engineered update-origins.js to understand what the hell the error means, as there's definitely nothing written about this in docs/CONTRIBUTING.md

@@ -221,7 +221,8 @@ export default {
},
'gitlab.com': {
url: '*://*.gitlab.com/*',
name: 'Gitlab'
name: 'Gitlab',
file: 'gitlab.js'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line was automatically added by update-origins.js script.

@glensc
Copy link
Contributor Author

glensc commented Jan 19, 2023

seems to me that CI job running update-origins is half-baked, existing integrations were not modified to add the header. also there's nothing documented about format of the header in CONTRIBUTING.md, and the update-origins.js itself has 0 comments about logic or what should it be doing, some obfuscation contest candidate there.

@glensc
Copy link
Contributor Author

glensc commented Jan 19, 2023

Should be rather trivial to write a script that:

  1. takes current origins.js
  2. assembles expected comment block for each integration
  3. writes that to each integration replacing existing block if it's there.
  4. git add -p to verify each change
  5. commit the changes

but given past slow and negative experience with this repo, I would not do that unless someone explicitly asks to do that.

also, the error that the script outputs, should guide the user to go to docs/CONTRIBUTING.md#file-header, and that # File header block should be written first.

@glensc
Copy link
Contributor Author

glensc commented Jan 19, 2023

Also, CI for update-origins is set up incorrectly. it runs in the context of the contributing user repository:

so, it doesn't appear at all under pull request:

probably another reason why other integrations lack the header. I just happened to read my email to see the CI failure.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant