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

2311-keycloak-impersonate #2402

Merged
merged 25 commits into from
Feb 4, 2025
Merged

2311-keycloak-impersonate #2402

merged 25 commits into from
Feb 4, 2025

Conversation

ojbravo
Copy link
Contributor

@ojbravo ojbravo commented Jan 29, 2025

Closes #2311

@ojbravo ojbravo marked this pull request as draft January 29, 2025 21:31
@ojbravo ojbravo marked this pull request as ready for review February 3, 2025 16:55
@@ -1,17 +1,20 @@
ARG NODE_VERSION='20.9.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this env variable is no longer used. Could this be integrated into line 5 to drive the node version of the Cypress image?

@@ -1,17 +1,20 @@
ARG NODE_VERSION='20.9.0'
ARG CYPRESS_VERSION='13.3.1'
ARG CYPRESS_VERSION='13.5.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this env variable is no longer used. Could this be integrated into line 15 to drive the version of the Cypress image?


# Set working directory
WORKDIR /hmda-frontend

USER root
RUN apt-get update && apt-get install -y curl nodejs npm microsoft-edge-stable
Copy link
Contributor

Choose a reason for hiding this comment

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

Is nodejs needed here if Node is already included in the cypress/browsers image?

#RUN apt-get install -y curl

COPY cypress/package.json ./
RUN npm install [email protected] --save-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

I see cypress already included in the cypress/package.json file. Could this install (Line 15) instead be managed by updating that Cypress version via the package file? Then Line 15 becomes npm install.


# Set working directory
WORKDIR /hmda-frontend

USER root
RUN apt-get update && apt-get install -y curl nodejs npm microsoft-edge-stable
#RUN apt-get install -y curl
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#RUN apt-get install -y curl

Copy link
Contributor

@meissadia meissadia left a comment

Choose a reason for hiding this comment

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

Approving since things are working as-is.

@ojbravo For Dockerfile cleanup, please create either a follow-up ticket and link it to #2311, or create a subtask in #2311 since the work is closely related.

@ojbravo ojbravo merged commit a2a629e into master Feb 4, 2025
1 of 2 checks passed
@ojbravo ojbravo deleted the 2311-keycloak-impersonate branch February 4, 2025 22:30
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.

[Cypress/Login.gov] Write tests for Login.gov login workflow
2 participants