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

feat: simplify example app's initial setup #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bfabio
Copy link
Contributor

@bfabio bfabio commented Jan 16, 2021

(upstreaming spid-express changes)

spid-testenv2 1.1.0 autogenerates the key & certificate and it
can also get a remote metadata at runtime, allowing us to avoid
having to manually save the metadata to a file.

The testenv GETs the metadata from the example app on the first
login attempt.

spid-testenv2 1.1.0 autogenerates the key & certificate and it
can also get a remote metadata at runtime, allowing us to avoid
having to manually saving the metadata to a file.

The testenv GETs the metadata from the example app on the first
login attempt.
@codecov
Copy link

codecov bot commented Jan 16, 2021

Codecov Report

Merging #65 (626c285) into master (56e2e28) will increase coverage by 0.79%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   66.26%   67.06%   +0.79%     
==========================================
  Files          11       11              
  Lines         750      759       +9     
  Branches      117      116       -1     
==========================================
+ Hits          497      509      +12     
+ Misses        252      249       -3     
  Partials        1        1              
Impacted Files Coverage Δ
src/config.ts 100.00% <ø> (ø)
src/strategy/spid.ts 37.20% <0.00%> (+0.25%) ⬆️
src/index.ts 64.19% <92.85%> (+4.77%) ⬆️
src/utils/response.ts 45.45% <0.00%> (+9.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ee11cc...5c6974d. Read the comment docs.

Copy link
Contributor

@balanza balanza left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, we really appreciate. I just left a couple of comments.

@@ -25,11 +25,12 @@ services:
- spid-testenv2

spid-testenv2:
image: italia/spid-testenv2:latest
image: italia/spid-testenv2:1.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why stick to a specific version and not use the latest available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's good practice to stick to a specific version for repeatability. A new major release could bring changes that break our assumptions on how the testenv behaves, such as an incompatible configuration format.

spid-testenv/config.yaml Outdated Show resolved Hide resolved
Co-authored-by: Daniele Manni <BurnedMarshal@users.noreply.github.com>
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