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

Feature/adding ci git actions #165

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

DanBezalelpx
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@ori-gold-px ori-gold-px left a comment

Choose a reason for hiding this comment

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

Lots of demo site things here can be removed and simplified -
(1) No need for all the templates, build/run scripts, and all the different configuration files. I say simplify this as much as possible!
(2) Remove all identifiers from the demo app (app IDs, tokens, etc.) - these should be included as repo secrets in GH, but never ever hard-coded in the repo.

@@ -0,0 +1,87 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lots of these files you don't need - for instance, there's no need to have a shared_config.json and a px_config.json and a config.json and a config.inc.json. Ultimately, all these are to generate one px_config.json file that's used for the sample site.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DanBezalelpx Still all 4 of these files - I don't think you need all of them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed most of them (only the unnecessary with safe remove

demo-site/servers/nodejs/config.json Outdated Show resolved Hide resolved
demo-site/templates/origin/origin_app.js Outdated Show resolved Hide resolved
demo-site/templates/test_endpoints/test_endpoints_app.js Outdated Show resolved Hide resolved
demo-site/scripts/build_docker.sh Outdated Show resolved Hide resolved
demo-site/scripts/build_docker.sh Outdated Show resolved Hide resolved
demo-site/scripts/run_docker.sh Outdated Show resolved Hide resolved
demo-site/templates/origin/Dockerfile Outdated Show resolved Hide resolved
demo-site/utils/cdn_deploy_tool_utils.js Outdated Show resolved Hide resolved
@DanBezalelpx DanBezalelpx force-pushed the feature/adding_ci_git_actions branch from 7f0c342 to a4e586b Compare June 26, 2023 18:22
@PerimeterX PerimeterX deleted a comment from ori-gold-px Jun 26, 2023
@PerimeterX PerimeterX deleted a comment from ori-gold-px Jun 26, 2023
@DanBezalelpx DanBezalelpx force-pushed the feature/adding_ci_git_actions branch from a4e586b to 072e114 Compare June 26, 2023 18:35
ci_files/build_cluster.sh Outdated Show resolved Hide resolved
.github/workflows/E2E_CI.yaml Outdated Show resolved Hide resolved
.github/workflows/E2E_CI.yaml Outdated Show resolved Hide resolved
.github/workflows/E2E_CI.yaml Outdated Show resolved Hide resolved
.github/workflows/E2E_CI.yaml Outdated Show resolved Hide resolved
.github/workflows/E2E_CI.yaml Show resolved Hide resolved
.github/workflows/E2E_CI.yaml Show resolved Hide resolved
.github/workflows/E2E_CI.yaml Outdated Show resolved Hide resolved
Comment on lines +8 to +22
extract_metadata:
runs-on: ubuntu-latest
name: Extract supported_features
outputs:
supported-features: ${{ steps.supported-features.outputs.value }}
steps:
- name: Checkout code
uses: actions/checkout@v3
- name: Setup Node.js
uses: actions/setup-node@v3
with:
node-version: '18.x'
- name: extract supported features
id: supported-features
run: echo "value=$(node -p -e "require('./px_metadata.json').supported_features?.join(' or ') || ''")" >> "$GITHUB_OUTPUT"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a separate job? Do we need it for something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We made it as a dependency in all of our new ci cd + i don't think it should be part of the CI action

Comment on lines +99 to +101
docker pull gcr.io/px-docker-repo/connecteam/mock-collector:1.0.2 && \
docker tag gcr.io/px-docker-repo/connecteam/mock-collector:1.0.2 localhost:5001/mock-collector:1.0.2 && \
docker push localhost:5001/mock-collector:1.0.2 && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to put the mock collector version in a variable or something? I'm thinking when we'll want to update it, finding it in all these places in the yaml will be annoying. Maybe we can add it to the px_metadata.json as well to have the versions of mock collector and tests so that we only need to update them in one place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're currently thinking of a way to include it as part of the tests set (for each tests version, we'll have a matching mock-collector version)
For now - we made it that way on every CI/CD and we're planning to address this issue across all ci/cd, I think that we can deploy the CI this way, and address it all together once we'll have a stable version.
@guyeisenbach FYI

Comment on lines +112 to +114
docker pull gcr.io/px-docker-repo/connecteam/enforcer-specs-tests:1.1.0 && \
docker tag gcr.io/px-docker-repo/connecteam/enforcer-specs-tests:1.1.0 localhost:5001/enforcer-spec-tests:1.1.0 && \
docker push localhost:5001/enforcer-spec-tests:1.1.0 && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

See above comment for mock collector

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same

@@ -0,0 +1,87 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DanBezalelpx Still all 4 of these files - I don't think you need all of them.

demo-site/scripts/create_px_configs.js Outdated Show resolved Hide resolved
demo-site/scripts/create_static_files.js Show resolved Hide resolved
Comment on lines +70 to +72
for (const [name, value] of Object.entries(req.headers)) {
res.setHeader(name, value);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was added so that the px-compromised-credentials header on the request would be transferred over to the response. It's not enough to transfer just that header, though, because it could change based on the enforcer configuration... but transferring ALL the headers to the response could be problematic? Ideally we would transfer only the px_compromised_credentials_header config value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DanBezalelpx I would modify this to be only the px-compromised-credentials header.

demo-site/servers/nodejs/app.js Show resolved Hide resolved
px_metadata.json Outdated Show resolved Hide resolved
@@ -0,0 +1,83 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think having this is unnecessary - or at least it's unnecessary to have the "enforcer_config" property here since the px_config.json file has the same info. I'd say choose one place to put this information - one source of truth.

@@ -0,0 +1,105 @@
//region imports
const fs = require('fs');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may not be necessary but it can stay because we still have to generate the static files based on the app_id and first_party configs so it's okay for now.

Comment on lines 17 to 25
const CDN_DEPLOY_TOOL_CONFIG_FILE_NAME = "cliConfig.json";
const CDN_DEPLOY_TOOL_RUN_CLI_COMMAND = "npm run cli";
const CDN_DEPLOY_TOOL_BUILD_COMMAND = "npm run build";

const PX_METADATA_FILE_NAME = "px_metadata.json";

const TEST_APP_CREDENTIALS_ENDPOINT = "/test-app-credentials";
const SUPPORTED_FEATURES_ENDPOINT = "/supported-features";
const CONFIG_ENDPOINT = "/config";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-evaluate if all these constants are necessary? Some of them (especially CDN deploy tool ones) may not be.

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.

3 participants