Skip to content

feat(epic-tutorials-ingestor): new heptraining ingestor and tests - #1383

Open
kennethrioja wants to merge 1 commit into
ElixirTeSS:masterfrom
kennethrioja:epic-ingestor
Open

kennethrioja wants to merge 1 commit into
ElixirTeSS:masterfrom
kennethrioja:epic-ingestor

Conversation

@kennethrioja

Copy link
Copy Markdown
Contributor

Summary of changes

  • Adding a new Heptraining::Ingestor
  • Amending app/models/source_filter.rb (last time modified by @eilmiv), I got a /code/app/models/source_filter.rb:22: warning: already initialized constant FILTER_DEFINITIONS ... /code/app/models/source_filter.rb:22: warning: previous definition of FILTER_DEFINITIONS was here and the suggested fix was to include it in the SourceFilter class, it didn't break tests afaik.
  • Moving CDS Video ingestor under heptraining_ingestors

Motivation and context

For HEPTraining to be able to scrape trainings from: https://eic.github.io/documentation/tutorials.html

Comments

It is useful to call other ingestors from one ingestor. I hope that one day we create a 'super ingestor' that reads the URL (which is almost always a prerequisite in the ingestors) and sends the job to the relevant ingestor. That would help admins to not choose the wrong ingestor among the 40+ current ingestors.

Checklist

  • I have read and followed the CONTRIBUTING guide.
  • I confirm that I have the authority necessary to make this contribution on behalf of its copyright owner and agree to license it to the TeSS codebase under the BSD license.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The advertised source cannot be parsed directly, and a cold ingestion exhausts GitHub’s unauthenticated API quota.

Review effort: Balanced
Findings: 2 High severity

Open (2)

response = open_url(url, raise: true)
return unless response

process_frontmatter(response.read)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ignore, I will use the source raw file which is not an html, but a md.

Comment on lines +67 to +70
ingestor = if github_url?(url)
Ingestors::GithubIngestor.new
elsif indico_url?(url)
Ingestors::IndicoIngestor.new

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know, the ingestor will need to wait a day more to ingest more GH resources. I'll keep it like that for now and accept the constraints.

@kennethrioja

Copy link
Copy Markdown
Contributor Author

@fbacall I addressed Copilot reviews (which were not useful here :( )

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants