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

TECH-156 - Update the Civic frontend canister #14

Merged
merged 27 commits into from
May 14, 2024

Conversation

TYRONEMICHAEL
Copy link
Contributor

No description provided.

@happyhackerbird happyhackerbird marked this pull request as ready for review May 14, 2024 12:16
dfx canister create --all
- name: Set env variables
run: |
export VITE_LOCAL_CIVIC_FRONTEND_CANISTER_ID=$(dfx canister id civic_canister_frontend)
Copy link
Member

Choose a reason for hiding this comment

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

If useful when running locally too I would extract this into a script

Copy link
Contributor

Choose a reason for hiding this comment

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

but should we leave it in the yaml ?

Copy link
Member

Choose a reason for hiding this comment

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

the script would be called from the ci yaml then like this

- name: Set env variables
      run: |
        . ./set-env-vars.sh

run: |
chmod +x ic-test-state-machine
cargo test --test integration_tests
- run: echo "🐧 This job's status is ${{ job.status }}."
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

the test or the output ?

Copy link
Member

@dankelleher dankelleher May 14, 2024

Choose a reason for hiding this comment

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

the output - the step will fail if the tests fail , or pass if they pass. The echo step will not be called if the tests fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

oops the echo is supposed to be a separate step actually. to print the status of the job.

@@ -0,0 +1,64 @@
# name: CI Build and Test (Mainnet setup)
Copy link
Member

Choose a reason for hiding this comment

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

Is this whole file commented out?

Copy link
Contributor

Choose a reason for hiding this comment

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

bc i thought we are writing a script for the mainnet deployment once off. and later we can add the CI for updating that canister on main. i will delete this file then.

@@ -118,11 +119,11 @@ impl Storable for IssuerConfig {

impl Default for IssuerConfig {
fn default() -> Self {
let derivation_origin = format!("http://{}.localhost:4943", ic_cdk::id().to_text());
let derivation_origin = format!("http://{}.icp0.io", ic_cdk::id().to_text());
Copy link
Member

Choose a reason for hiding this comment

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

Should this be parameterised?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like passed from the workflow ? But this is the default implementation for the issuer, resolving to mainnet config

Copy link
Member

Choose a reason for hiding this comment

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

it is using http (not https) is that correct?

What happens if we want to run everything locally, (issuer, RP, Internet Identity). Do we need to change this? In general I wouldn't expect URLs to be stored in canister code hard-coded. Is it good practice?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually i fixed this in another branch im working on on the smart contract. its https.

default will only be called if we deploy the canister with no init arguments (in the current deploy script we are always passing arguments). but it has to implement this trait Default

Copy link
Contributor

Choose a reason for hiding this comment

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

once this is merged into develop i can apply my smart contract commits on top. so i would leave the http here

const civicFrontendCanisterUrl = `http://${civicFrontendCanisterId}.${host}`;

// This is for demo purposes but should be replaced with a more secure method
const dummyCivicSampleKey = new Uint8Array([
Copy link
Member

Choose a reason for hiding this comment

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

This is for issuing credentials? Can we make it an env var if so?

@happyhackerbird happyhackerbird merged commit 76a8dc2 into develop May 14, 2024
4 checks passed
happyhackerbird added a commit that referenced this pull request May 14, 2024
* TECH-156 - Update the Civic frontend canister

* TECH-156 - Get Civic issuing Credential canister to work

* alternative origin file with placeholder

* configure civic deploy script (including alternative origins)

* fix derivationOrigin in civic frontend login

* fixes for derivationOrigin

* fix the alternative_frontend url in the civic-deploy script

* TECH-156 - Add initial commit for the relying canister frontend application

* ci for local setup

* fix yaml

* use npm instead of yarn

* test fixes & mainnet ci

* TECH-156 - Fix issue with retrieving the credential

* snake case for canister names; fix CI

* Update README.md

* env-vars script & fixes
---------

Co-authored-by: Lilly Guo <[email protected]>
Co-authored-by: Lilly <[email protected]>
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