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

Move to Dockerfile installation #32

Merged
merged 18 commits into from
Aug 31, 2023

Conversation

tyler36
Copy link
Collaborator

@tyler36 tyler36 commented Aug 23, 2023

Fixes #31

@tyler36 tyler36 force-pushed the issue31_Move-to-Dockerfile-installation branch from cfce2d2 to bd3e3e8 Compare August 23, 2023 02:03
@tyler36 tyler36 requested a review from rfay August 23, 2023 02:11
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

This is awesome. A few problems with copy-pasta, but it's great.

The overall problem of how to manage custom configuration without breaking future ddev get ddev/ddev-cron and also making ddev get --remove work successfully has possibilities. In add-ons I've been using unmanaged files (non-#ddev-generated) like docker-compose.redis-extra.yaml for example as overrides. Not sure how easy that is to do here but I think you'll get it.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 24, 2023

Wow ... Not sure where my head was that day. Sorry for the low quality.

@tyler36 tyler36 force-pushed the issue31_Move-to-Dockerfile-installation branch from a20f135 to 4ca42b3 Compare August 25, 2023 07:58
@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 25, 2023

Did some research and had a rethink.

  • No default job is present OOTB
  • Update now allows multiple *.cron jobs
  • Examples updated to show specific steps.

@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 25, 2023

I suspect the "install from release" test are failing because they don't the content from this PR, no?

@rfay
Copy link
Member

rfay commented Aug 25, 2023

Yes, you can either ignore the "install from release" tests or temporarily disable them.

@tyler36 tyler36 requested a review from rfay August 28, 2023 00:08
@rfay
Copy link
Member

rfay commented Aug 28, 2023

Test with

ddev get https://github.com/tyler36/ddev-cron/tarball/issue31_Move-to-Dockerfile-installation

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I tried this out and it looks great. Some suggestions along the way.

  • I think it would be good to add info about timezone: configuration here because people will get confused about what time things happen without that
  • Mentioning ddev exec crontab -l may help people to see what's configured.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tyler36 tyler36 force-pushed the issue31_Move-to-Dockerfile-installation branch 2 times, most recently from d4bd794 to 4cf3aea Compare August 29, 2023 01:06
@tyler36 tyler36 force-pushed the issue31_Move-to-Dockerfile-installation branch from 4cf3aea to 6986d00 Compare August 29, 2023 01:34
@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 29, 2023

OK ... merged the suggested changes.

Also fixed the issue with running multiple jobs so should be OK now.

@tyler36 tyler36 requested a review from rfay August 29, 2023 01:35
@rfay
Copy link
Member

rfay commented Aug 29, 2023

OH, now I remember why IS_DDEV_PROJECT is critical in the context of cron. Cron doesn't pass any environment variables along. So for any CMS type that has DDEV-generated settings, like Drupal or TYPO3 but probably not Laravel? the settings won't be executed without it. Sorry for forgetting this.

@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 29, 2023

Thanks. I thinks that everything now, or did I miss something?

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Several suggestions didn't get committed or discussed, please return to those.

The one thing that might make it easier for people would be to provide the main examples as examples in the .ddev/web-build. Maybe the drupal and typo3 examples, for example, just like you do with the simple time log.

Thanks for this!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@tyler36 tyler36 force-pushed the issue31_Move-to-Dockerfile-installation branch from 38c6708 to 6986d00 Compare August 30, 2023 00:20
tyler36 and others added 2 commits August 30, 2023 09:20
Co-authored-by: Randy Fay <[email protected]>
Co-authored-by: Randy Fay <[email protected]>
@tyler36 tyler36 force-pushed the issue31_Move-to-Dockerfile-installation branch from 2d2c393 to 5aed29f Compare August 30, 2023 00:28
@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 30, 2023

Mentioning ddev exec crontab -l may help people to see what's configured.

I replace crontab -e with crontab -l in the example. -e asks about selecting an editor, -l simply outputs the contents

@tyler36 tyler36 requested a review from rfay August 30, 2023 00:45
@rfay
Copy link
Member

rfay commented Aug 30, 2023

Sorry, we're getting lost here, and the review page is getting too cluttered and I get lost in it now.

Here's what the README has right now:
https://github.com/ddev/ddev-cron/blob/642fe9ab2c94cfd4ad7f2ac0d5e3c40575954c58/README.md

This is one of the things I was trying to get rid of because I don't believe people will ever need to edit it:

image

Other than thinking we shouldn't confuse people with that one, I think everything is fine to go.

Sorry it's been so difficult to get through this, but I sure appreciate this good work.

@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 30, 2023

Appologies for the very messy work flow on this. The VSCode extension seemed inconsistent on this repo. And my fingers were too fat at other times.

ddev-cron-readme

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks, looking great. Pull when you're ready.

I don't think that github "Files changed" page was helping us much. It got all messy and you couldn't tell what was where.

@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 31, 2023

Thank you.

We got there in the end.

@tyler36 tyler36 merged commit d417d98 into ddev:main Aug 31, 2023
0 of 2 checks passed
@tyler36 tyler36 deleted the issue31_Move-to-Dockerfile-installation branch August 31, 2023 04:35
@tyler36
Copy link
Collaborator Author

tyler36 commented Aug 31, 2023

All tests from this PR passed after merge and 1.5.0 (which included this) was released.

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.

Move to Dockerfile installation
2 participants