-
Notifications
You must be signed in to change notification settings - Fork 237
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
chore: create preview environments on every PR #639
Conversation
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.
This looks very interesting at a glance. I'd wait to make sure the previews work before approving this, so I'll allow the CI runs and maybe this will have to wait for #637
@waveywaves seems you also need to run |
@BrunoBernardino done with |
@BrunoBernardino you can see what the preview looks like over here https://app.uffizzi.com/github.com/waveywaves/padloc/pull/3 By any chance we can get on a call so I can walk you through the preview and the uffizzi platform which you should be able to use for your PRs to padloc ? |
@waveywaves it looks good at a glance, though we'd need a way to see the preview server's logs in order to, for example, sign up (to see the verification code). As for a call, I'd rather do the process async, so if there's a specific landing page, document, or video I could take a look at, that'd help look at it, though Padloc doesn't have the budget/need to pay for such a tool. |
The server logs url is shared alongside the preview URL in the comment.
I understand. Check the docs over here https://docs.uffizzi.com/ I am ccing @MaKleSoft as we noticed that you signed up on the platform, we have removed the paywall for @MaKleSoft so you can use uffizzi without any issues. Can do the same for you as well @BrunoBernardino, let me know once you signup to https://app.uffizzi.com using your github account. |
d4bf572
to
90bb7e7
Compare
@waveywaves I'm getting multiple notifications a day for this PR, apparently because you keep force-pushing. Are you actually updating the PR or is there a different reason? |
@MaKleSoft I just finished the last push, this was for fixing this PR based on #639 (comment) comment. Let me know what you think about this PR though. |
@waveywaves should we be seeing a comment with the preview URL + logs here? |
@BrunoBernardino Not on this PR, but in all subsequent PRs. The changes from this PR need to be on your default branch to trigger the deployment. As this PR is merged I will open another PR to test the previews. |
@waveywaves Ok. I'll wait for @MaKleSoft to give the final approval. Meanwhile, please don't force-push commits as it makes it impossible to review changes between commits, and so I have to review everything everytime to make sure nothing wrong came through. |
docker-compose.uffizzi.yml
Outdated
@@ -0,0 +1,56 @@ | |||
version: "3" |
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.
Can this file be moved to uffizzi
's directory?
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.
done !
@MaKleSoft just waiting on your review, here. |
Looking forward to it ! Will test it as soon as it is merged. |
@MaKleSoft any updates on this ? |
@BrunoBernardino @MaKleSoft Hi guys ! Let me know if you have any queries regarding the service. I am pleased to share that recently we have had the opportunity to support a lot of other brilliant open source projects which you can see here. Let me know what you think. Also, once the PR is merged I will be testing this ASAP to understand how well the changes work, let me know if this makes sense to you. |
@waveywaves @MaKleSoft should be the one to finally merge this, but it looks good to me. |
@MaKleSoft @BrunoBernardino friendly ping - checking in to see if you want to move forward with us otherwise we'll close it out. |
@jpthurman sorry about the radio silence! I'm pretty swamped right now and don't really have time to review this properly. But the truth is that although I can see the benefits from something like this, I don't really feel comfortable involving another third party in our build process, especially since this could potentially open up an attack surface for supply chain attacks. I definitely don't want to merge this without giving it some proper thought and considering all the possible implications, and honestly I currently don't see this adding enough value to justify the effort. We might consider a self-hosted version of this in the future, but right now I'd rather close this PR and revisit it later. Sorry! |
Thanks for the response @MaKleSoft To address your security concerns, the loop for Previewing for Uffizzi is isolated from any infrastructure you own. The built images are pushed to our ephemeral container registry and they are pulled from there when a preview is deployed onto our infrastructure. The images and environments have an ephemeral life-cycle or a planned deletion. There's several projects using our service in the same way as this PR outlines. We purpose designed a 2-stage workflow so outside contributors can get previews securely. |
This PR adds github workflows which will trigger uffizzi preview environments for PRs to this repo. A PoC has been created at this PR. Once this PR is merged, you should be able to see comments like these on PRs to this repo. These comments are created after a preview env has been deployed for the PR.
fixes #613