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

Added DENO_DEPLOYMENT_ID env variable #155

Merged
merged 8 commits into from
May 7, 2023

Conversation

cdoremus
Copy link
Contributor

@cdoremus cdoremus commented May 3, 2023

Added DENO_DEPLOYMENT_ID environmental variable needed for Docker deployment of a Deno
Fresh app for caching to work properly (see https://fresh.deno.dev/docs/concepts/deployment#docker).

README.md Outdated Show resolved Hide resolved
@lambtron
Copy link
Collaborator

lambtron commented May 3, 2023

am i understanding the directions correctly RE: docker deployment that for each deployment, you have to run

git rev-parse HEAD

and then copy and paste that into .env file as DENO_DEPLOYMENT_ID?

maybe we suggest using GitHub Actions, in which it will grab the git rev-parse HEAD and assign it to DENO_DEPLOYMENT_ID before it builds/ tags the Docker image, and deploys it to whichever VPS.

what do you think?

iuioiua and others added 2 commits May 4, 2023 09:10
@cdoremus
Copy link
Contributor Author

cdoremus commented May 4, 2023

maybe we suggest using GitHub Actions, in which it will grab the git rev-parse HEAD and assign it to DENO_DEPLOYMENT_ID before it builds/ tags the Docker image, and deploys it to whichever VPS.

what do you think?

As we talked about on Discord, this is something that we would implement in the future.

@iuioiua iuioiua requested a review from lambtron May 4, 2023 22:17
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 267 to 273

```sh
# get the SHA1 commit hash of the current branch
git rev-parse HEAD
```

Make sure this command is run and value upgraded
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can prob remove this part given the re-organization of the information in the paragraph prior

Copy link
Contributor Author

@cdoremus cdoremus May 5, 2023

Choose a reason for hiding this comment

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

@lambtron I agree that the last line (273) needs to be removed, but the git rev-parse HEAD command should remain as the previous paragraph points to it. Don't you agree?

@lambtron
Copy link
Collaborator

lambtron commented May 4, 2023

LGTM after those changes!

cdoremus and others added 2 commits May 4, 2023 22:45
@iuioiua
Copy link
Contributor

iuioiua commented May 5, 2023

Could you please fix the CI errors?

@iuioiua
Copy link
Contributor

iuioiua commented May 5, 2023

Is this PR ready to merge?

@cdoremus
Copy link
Contributor Author

cdoremus commented May 7, 2023

Is this PR ready to merge?

Yes

@cdoremus
Copy link
Contributor Author

cdoremus commented May 7, 2023

Could you please fix the CI errors?

Done in e180b16

@iuioiua iuioiua merged commit 611716d into denoland:main May 7, 2023
@iuioiua
Copy link
Contributor

iuioiua commented May 7, 2023

Thanks for this, @lambtron and @cdoremus!

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