-
Notifications
You must be signed in to change notification settings - Fork 43
feat: support codex cli #281
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
base: main
Are you sure you want to change the base?
Conversation
@matifali PR ready for review |
|
||
## Prerequisites | ||
|
||
- You must add the [Coder Login](https://registry.coder.com/modules/coder-login/coder) module to your template |
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 we instead use the module inside the module? I know we have the same requirement in other modules too.
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.
@matifali So, instead of the user adding this, we pre-include it in our module ?
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.
Yes. Needs to test though how it works.
@hugodutka @DevelopmentCats, please review and let me know if this needs any changes :) |
@hugodutka thanks for the review, committed the suggested changes. |
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.
Great work, just some nits. I expect we'll merge after the next review.
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
Co-authored-by: Hugo Dutka <dutkahugo@gmail.com>
@hugodutka @matifali made the changes. |
@35C4n0r when I ran the module in my workspace I got the following error:
Codex has probably been updated since you last tested if fully. Could we add a test to check that codex works end to end, meaning that it starts successfully after installing the latest version? |
@hugodutka will do so, looks like there have been 9 minor releases since I raised this PR, I developed in |
@hugodutka how should we handle this, should we give a base minimum to maximum version ? They haven't released a stable/major version yet, and will continue to push breaking changes regularly. |
I'd just add a test that installs the latest version and checks if it starts. The test will notify us if they push a breaking change later. |
makes sense, the tests fail now @hugodutka added the test. |
These are the minimal changes that would be needed to support |
Closes #236
/claim #236
Description
https://www.loom.com/share/4a39d1c2e2324cb2b6f90c1ad6893602?sid=d5211d2c-a2cd-41ae-8580-15ac686756f2
Type of Change
Module Information
Path:
registry/[namespace]/modules/[module-name]
New version:
v1.0.0
Breaking change: [ ] Yes [ ] No
Testing & Validation
bun test
)bun run fmt
)Related Issues