Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add E2E tests for chromium and firefox #1121
feat: add E2E tests for chromium and firefox #1121
Changes from all commits
bf8646e
00d14ea
5a933cd
6503c92
d8c6603
963e571
416cb1a
2807670
ba30b87
4bf55b6
25a4760
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
we should eventually upload the built docker image (
ipfs-companion-release-build
) and reuse that if possible.. (hash of source or non-docker build artifacts to determine if we want to use the same image?)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.
I did a write-up recently on docker publishing/caching in CI for build speed-up. See https://filecoinproject.slack.com/archives/C03KLC57LKB/p1673517809976679?thread_ts=1673468796.401379&cid=C03KLC57LKB
I'd say it's a little bit beyond the scope of this PR though.
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.
I created #1132 for this
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.
Note that we should be able to set hostname and domain name for these containers, which would make addressing https://github.com/ipfs/ipfs-companion/pull/1121/files#r1053853150 much easier. We would know the hostname, protocol, and port
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.
I'm not sure how exactly setting
hostname
anddomainname
works. Could you elaborate on how they could affect the setup?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.
Are you asking about how hostname and domainname are set inside the container? I'm not fully aware of the implementation details but I believe the etc/hostname file (and docker configs) are updated appropriately.
Ultimately, setting domainname and hostname would allow you to address the CORS concerns brought up by @whizzzkid, because you would be able to specify the origins set via hostname/domainname and port.
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.
I got confused a bit and though these were global settings for the compose file 🤦
Back to the matter, we're already "using" hostname. If the value is not specified, then the hostname defaults to the service name. We already use this fact to communicate between services. E.g. that's how browsers know they can find Kubo at http://kubo.
Domainname, I'm not so familiar with, but apparently it's not that useful - https://stackoverflow.com/questions/70741780/what-is-the-domainname-option-used-for-in-docker
Anyway, I tried to be more explicit about access control and limit the origins to
http://chromium
andhttp://firefox
but, apparently, that's not enough.You can now set access control values through env - https://github.com/ipfs/ipfs-companion/blob/25a4760138ac21fdfdf198a6476a88b92bd09c4c/ci/access-control-allow-all.sh. This should allow for easier experimentation in the future.
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.
I created an issue for this at #1130
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.
+1. thanks for this
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.