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.

Sign up for GitHub

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

Jump to bottom

Making parser service work #2846

Merged
merged 1 commit into from
Feb 22, 2022
Merged

Making parser service work #2846

merged 1 commit into from
Feb 22, 2022

Conversation

TueeNguyen
Copy link
Contributor

@TueeNguyen TueeNguyen commented Feb 6, 2022 •
edited
Loading

Issue This PR Addresses

Fixes #2111
This is a big change so I'd rather have it looked at early

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Steps to test the PR

  1. pnpm install in root
  2. pnpm services:start redis elasticsearch traefik posts
  3. npm run dev and go to localhost:8000 or look at the terminal log
    Notes: I have not tested this for Youtube feeds, will edit this with test result

Next steps

Should I open small issues for these steps?

  1. Open a proper PR or keep this
  2. Add unit and e2e tests
  3. Dockerize parser service

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Feb 6, 2022

@Kevan-Y Kevan-Y added area: microservices type: enhancement New feature or request type: test Creation and development of test labels Feb 6, 2022
@TueeNguyen TueeNguyen added this to the 2.7 Release milestone Feb 6, 2022
JerryHue
JerryHue reviewed Feb 8, 2022
View reviewed changes
Copy link
Contributor

@JerryHue JerryHue left a comment

Choose a reason for hiding this comment

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

I think the dockerization can be done in a separate PR, we want to focus on migrating all the changes from the legacy backend into the parser service, which is already a big task. The testing can be done on a separate PR, as well.

src/api/parser/README.md Outdated Show resolved Hide resolved
src/api/parser/src/data/feed.js Outdated Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Feb 9, 2022

Can you rebase for the lock file changes?

@menghif menghif requested a review from dbelokon February 17, 2022 13:11
humphd
humphd requested changes Feb 17, 2022
View reviewed changes
src/api/parser/README.md Show resolved Hide resolved
src/api/parser/package.json Outdated
"normalize-url": "6.0.1",
"rss-parser": "3.12.0",
"sanitize-html": "2.5.3",
"set-interval-async": "2.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of this and use https://nodejs.org/api/timers.html#timerspromisessetintervaldelay-value-options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using setInterval and clearInterval from timers/promise works, I removed the package

src/api/parser/src/utils/__mocks__/indexer.js
@@ -0,0 +1,64 @@
// Mock storage for our es data
Copy link
Contributor

Choose a reason for hiding this comment

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

@rclee91 @JiaHua-Zou ping?

@humphd humphd modified the milestones: 2.7 Release, 2.8 Release Feb 18, 2022
@humphd
Copy link
Contributor

humphd commented Feb 18, 2022

@TueeNguyen want to rebase this so it picks up the changes we made last night?

@TueeNguyen TueeNguyen requested a review from humphd February 18, 2022 17:31
@TueeNguyen TueeNguyen marked this pull request as ready for review February 18, 2022 17:33
@manekenpix @TueeNguyen
Making parser service work:
6518596 
- Update README.md
- Revert some docker, config changes
- Removed unncessary files (hash.js, redis.js, elastic.js)
- Change imports to use Satellite's (hash, redis, elastic, logger)
- Add remove-empty-anchor to html/index.js
- Remove some dependencies in package.json
- Update lock file
humphd
humphd approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

OK, I'm tempted to take this, and let us move forward on tests.

Great work, and than you for not giving up easily.

src/api/parser/src/utils/html/sanitize.js
a: ['href'],
},
allowedIframeHostnames: [
'www.youtube.com',
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a PR in flight right now that alters this file, let's not forget to update this too or later (@HyperTHD).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remember

@humphd humphd mentioned this pull request Feb 22, 2022
Merged
@aserputov aserputov merged commit 387350f into Seneca-CDOT:master Feb 22, 2022
@TueeNguyen
Copy link
Contributor Author

Thanks everyone, I can write tests for it now :D

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

@aserputov aserputov aserputov approved these changes

@humphd humphd humphd approved these changes

@tpmai22 tpmai22 Awaiting requested review from tpmai22

@JerryHue JerryHue Awaiting requested review from JerryHue

@manekenpix manekenpix Awaiting requested review from manekenpix

@RC-Lee RC-Lee Awaiting requested review from RC-Lee

@dbelokon dbelokon Awaiting requested review from dbelokon

Assignees

@TueeNguyen TueeNguyen

Labels
area: back-end area: microservices type: enhancement New feature or request type: test Creation and development of test
Projects
None yet
Milestone
2.8 Release
Development

Successfully merging this pull request may close these issues.

Write tests and enable Parser service
9 participants
@TueeNguyen @humphd @manekenpix @RC-Lee @JerryHue @aserputov @dbelokon @sirinoks @Kevan-Y