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

Fixes #1870, port hash function into Satellite #1913

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

Metropass
Copy link
Contributor

Issue This PR Addresses

Fixes #1870

Type of Change

Ported Hash Functions from Satellite onto Telescope, removed hash.js

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

Description

Discussed in #1870 , We ended up porting our hash function onto Satellite, and removed hash.js on both /api and /backend

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)

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.

The changes to the auth service are good, but let's undo the ones to src/backend and test/, since that code doesn't use microservices.

Our goal is to replace the current backend (i.e., delete it) with the microservices once they are done, similar to what we did with next and gatsby.

package.json Outdated
@@ -53,6 +53,7 @@
"dependencies": {
"@elastic/elasticsearch": "7.11.0",
"@elastic/elasticsearch-mock": "0.3.0",
"@senecacdot/satellite": "1.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to use Satellite in the monolithic back-end, only in the microservices. Let's undo this.

@@ -1,5 +1,5 @@
const User = require('./user');
const hash = require('./hash');
const satellite = require('@senecacdot/satellite');
Copy link
Contributor

Choose a reason for hiding this comment

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

Just pull in the function you need.

const { hash } = require('@senecacdot/satellite');

@@ -1,6 +1,6 @@
const Feed = require('./feed');
const User = require('./user');
const hash = require('./hash');
const satellite = require('@senecacdot/satellite');
Copy link
Contributor

Choose a reason for hiding this comment

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

No to this change (only in microservices).

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.

Bring back src/backend/data/hash.js and this is probably good to go.

@Metropass
Copy link
Contributor Author

Bring back src/backend/data/hash.js and this is probably good to go.

Just did, and I rebased, so it should be good to go. Are there any other modules that we could port onto Satellite?

humphd
humphd previously approved these changes Mar 12, 2021
@Metropass Metropass requested a review from tonyvugithub March 12, 2021 17:22
birtony
birtony previously approved these changes Mar 12, 2021
@Metropass Metropass merged commit 2e6fe53 into Seneca-CDOT:master Mar 12, 2021
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.

Figure out how to share our hasing algo across microservices
3 participants