-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: introduce the logic to list/promote/stop squids #5
base: main
Are you sure you want to change the base?
Conversation
src/controllers/auth-middleware.ts
Outdated
} | ||
|
||
const token = authHeader.slice(7); // Extract token after 'Bearer ' | ||
const expectedToken = process.env.AUTH_TOKEN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about grabbing this environment variable from the config component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auth-middleware removed entirely
src/ports/squids/component.ts
Outdated
import { Network } from "@dcl/schemas"; | ||
|
||
const AWS_REGION = "us-east-1"; | ||
const CLUSTER_NAME = process.env.CLUSTER_NAME || "dev-main"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about grabbing this environment from the config component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, done!
src/ports/squids/queries.ts
Outdated
-- Fetch the old schema name from the squids table | ||
SELECT schema INTO old_schema_name | ||
FROM squids | ||
WHERE name = 'marketplace'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be fixed to marketplace
?
src/ports/squids/component.ts
Outdated
} | ||
} | ||
|
||
async function stop(serviceName: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not wrong, by exporting the stop function, the lifecycle mechanism of the components will run it upon termination. Would you mind checking if we need to change the name of the funcion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, renamed!
src/controllers/auth-middleware.ts
Outdated
next: () => Promise<IHttpServerComponent.IResponse> | ||
): Promise<IHttpServerComponent.IResponse> { | ||
// Validate Authorization header | ||
const authHeader = context.request.headers.get("Authorization"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're hardcoding the bearer token in one of our static UIs, we must take into consideration that those UIs are packaged and deployed to NPM before being uploaded to the CDN, carrying the token with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auth middleware removed entirely!
.github/workflows/node.yml
Outdated
if: ${{ always() }} | ||
- name: test | ||
run: npm run test | ||
- name: integration test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to do integration tests over the promotion process, but, as of today, I would remove the integration tests step that will make the job fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed for the moment!
.github/workflows/deployer.yml
Outdated
id: deploy | ||
uses: decentraland/dcl-deploy-action@main | ||
with: | ||
dockerImage: "quay.io/decentraland/squid-management-server-:${{ inputs.tag }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed dash: "quay.io/decentraland/squid-management-server:${{ inputs.tag }}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, done!
.env.default
Outdated
|
||
HTTP_SERVER_PORT=5001 | ||
HTTP_SERVER_HOST=0.0.0.0 | ||
AWS_CLUSTER_NAME=dev-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why dev-main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to just pick a name, as an example, I changed to blank but it's not that important IMO
src/ports/squids/component.ts
Outdated
config: IConfigComponent; | ||
}): Promise<ISquidComponent> { | ||
const cluster: string = | ||
(await config.getString("CLUSTER_NAME")) || "dev-main"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why dev-main here? could we fail if CLUSTER_NAME is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to requireString
to ensure it has value or it explodes.
src/ports/squids/component.ts
Outdated
|
||
const serviceArns = servicesResponse.serviceArns || []; | ||
const squidServices = serviceArns.filter((arn) => | ||
arn.includes("squid-server") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could it be "-squid-server-" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid which cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make sure there is a project name before and a server A or B afterwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also to avoid case: "squid-servers-a" with extra "s"
This PR create the endpoints to:
/list
all the squid services that have been started and their statuses.and also adds some functions for the feature to :
stop
an indexerpromote
an indexerThis two endpoints entrypoints will be added in a following up PR.