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

Adds Initial Service to Get Posts #1838

Merged
merged 14 commits into from Mar 12, 2021
Merged

Adds Initial Service to Get Posts #1838

merged 14 commits into from Mar 12, 2021

Conversation

HyperTHD
Copy link
Contributor

@HyperTHD HyperTHD commented Feb 26, 2021

Issue This PR Addresses

This PR address #1735

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

This PR is a work-in-progress, meant to work on adding a Post Microservice. This microservice parses posts from user's feeds and returns them. There's a path that returns the 10 latest posts, and you can also retrieve information about a specific post using it's ID.

This PR includes files that are being used by other microservices. I recommend getting those PRs in first before this one so that I can update the docker-compose file to use those services. This would allow me to remove the files those services used and only keep what I need for my microservice. In particular, this would be the User Microservice, The Post parsing microservice (#1828), and possibly the Feed Parsing microservice (#1833).

UPDATE: Screenshot of successful loading of this route along with network tab open as proof
Posts-Microservice

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)

@HyperTHD
Copy link
Contributor Author

This PR is not tested btw. I'm still working on the Docker related stuff. I will post screenshots of the output when I get it working

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.

I did a first read through and left some feedback.

There's a lot of code in here that can be removed and done in #1828 by @c3ho (e.g., all the feed handling stuff).

Eventually I want to have the feed service use this service to get/set/update/delete posts.

src/api/posts/server.js Outdated Show resolved Hide resolved
src/api/posts/src/bin/article-error.js Outdated Show resolved Hide resolved
src/api/posts/src/routes/posts.js Show resolved Hide resolved
src/api/readme.md Outdated Show resolved Hide resolved
@HyperTHD HyperTHD self-assigned this Mar 1, 2021
@HyperTHD HyperTHD added area: back-end area: CI/CD Continuous integration / Continuous delivery area: deployment Production or Staging deployment area: microservices labels Mar 1, 2021
@HyperTHD HyperTHD added this to In progress in Microservice Architecture Port via automation Mar 1, 2021
@HyperTHD HyperTHD linked an issue Mar 1, 2021 that may be closed by this pull request
@HyperTHD
Copy link
Contributor Author

HyperTHD commented Mar 3, 2021

UPDATE:

  • I added the docker code to create the posts image. I may have done it wrong, but I needed to include redis and elasticsearch inside the docker-compose.yml file since it needs both to function.
  • I added the redis and elasticsearch information to all 3 env example files for reason specified above.
  • I modified the redis and elasticsearch host names so that they use the container names. Since we're running them off the container instead of localhost, it makes the most sense to do this.
  • Lastly, I refactored the code to remove the bin folder completely.

Running the posts service atm loads the right page, however, NO POSTS ARE LOCATED AND YOU GET AN EMPTY ARRAY AS A RESULT. I feel this is because of the changed host names, since the redis data is locally served instead of via the cloud. I'm not too sure about this one. Otherwise, I feel good about this service and can move to add in the extra routes.

As for adding test cases for this microservice, I plan on filing another issue to handle that. I want this PR strictly for porting this microservice over

@HyperTHD HyperTHD requested a review from humphd March 3, 2021 04:23
src/api/docker-compose.yml Outdated Show resolved Hide resolved
- ${REDIS_PORT}:${REDIS_PORT}
command: ['redis-server', '--appendonly', 'yes']
volumes:
- ../../../redis-data:/data
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder where we should put volumes related to the microservices. The root is one option for sure, but maybe we should have a dedicated directory where we can find these things. cc @manekenpix, @raygervais.

Copy link
Member

@manekenpix manekenpix Mar 3, 2021

Choose a reason for hiding this comment

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

@humphd this is how we currently do it on prod and dev:

Directory Structure

src/api/docker-compose.yml Outdated Show resolved Hide resolved
src/api/posts/src/processHTML/dom.js Outdated Show resolved Hide resolved
@manekenpix
Copy link
Member

I need some clarification for this PR. I thought this microservice was going to answer requests (serving posts), but I see a lot of code here related to processing feeds and posts. Shouldn't all that be part of a different microservice?

@HyperTHD @humphd

@humphd
Copy link
Contributor

humphd commented Mar 4, 2021

@manekenpix I left a similar review comment, a lot of this code can get deleted.

@HyperTHD
Copy link
Contributor Author

HyperTHD commented Mar 4, 2021

@manekenpix @humphd The main problem with removing that code is that there's no way for me to really know if the service works or not. The files related to my service also require the other files needed for feeds as well. Since we don't have those other services up, I can't realistically remove those extra files without breaking CI

@raygervais
Copy link
Contributor

Why not test using mock services to pass in hypothetical data @HyperTHD ?

humphd
humphd previously requested changes Mar 12, 2021
src/api/docker-compose.yml Outdated Show resolved Hide resolved
src/api/env.development Outdated Show resolved Hide resolved
src/api/posts/Dockerfile Outdated Show resolved Hide resolved
src/api/posts/package.json Outdated Show resolved Hide resolved
src/api/posts/package.json Outdated Show resolved Hide resolved
src/api/posts/package.json Outdated Show resolved Hide resolved
src/api/posts/package.json Outdated Show resolved Hide resolved
src/api/posts/test/posts.test.js Outdated Show resolved Hide resolved
@HyperTHD HyperTHD merged commit e4904a5 into Seneca-CDOT:master Mar 12, 2021
Microservice Architecture Port automation moved this from In progress to Done Mar 12, 2021
@HyperTHD HyperTHD deleted the issue-1735 branch April 7, 2021 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: back-end area: CI/CD Continuous integration / Continuous delivery area: deployment Production or Staging deployment area: microservices
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Creating a Post microservice
5 participants