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

Enable usage inside GitHub Codespaces #355

Closed
wants to merge 11 commits into from
Closed

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Nov 13, 2024

What's changing

This P.R. adds a new file .devcontainer/codespaces/devcontainer.json that allows to run and test Luminator inside GitHub Codespaces

How to test it

Create a codespace from the GitHub UI using New with options:

Screenshot 2024-11-13 113320

Make sure to select this branch and the Lumigator config:

image

Additional notes for reviewers

By default GitHub uses .devcontainer/devcontainer.json when creating a codespace. Since the existing one is not meant to be used in codespaces, perhaps it could be moved to a subfolder like .devcontainer/local/devcontainer.json?

I already...

  • added some tests for any new functionality
    Went through the full quickstart inside a codespace:

Screenshot 2024-11-13 113024
Screenshot 2024-11-13 113036

  • updated the documentation
  • checked if a (backend) DB migration step was required and included it if required

format=experiment failed with:

```
{
  "detail": [
    {
      "type": "enum",
      "loc": [
        "body",
        "format"
      ],
      "msg": "Input should be 'job'",
      "input": "experiment",
      "ctx": {
        "expected": "'job'"
      }
    }
  ]
}
```

- Use sample_data in the example.
@daavoo daavoo self-assigned this Nov 13, 2024
@peteski22 peteski22 requested a review from agpituk November 13, 2024 16:45
Copy link
Contributor

@peteski22 peteski22 left a comment

Choose a reason for hiding this comment

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

By default GitHub uses .devcontainer/devcontainer.json when creating a codespace. Since the existing one is not meant to be used in codespaces, perhaps it could be moved to a subfolder like .devcontainer/local/devcontainer.json?

I think that sounds like a good idea, I think the only place that would need updating might be the actual contents of the devcontainer.json as it has relative references to where to find the docker 'stuff'. Do you think this should be done in a follow-up PR?

@peteski22 peteski22 self-requested a review November 14, 2024 12:42
@daavoo
Copy link
Contributor Author

daavoo commented Nov 14, 2024

I think that sounds like a good idea, I think the only place that would need updating might be the actual contents of the devcontainer.json as it has relative references to where to find the docker 'stuff'. Do you think this should be done in a follow-up PR?

Since I am not really using the current .github/devcontainer.json it'd be better to consult with the rest of the team who might be using it.

TBH, having the codespaces one as default is just a convenience. How to launch a codespace to test lumigator and how to select the correct devcontainer can be explained in the docs

@daavoo
Copy link
Contributor Author

daavoo commented Nov 20, 2024

Since it is not clear if this direction makes sense for the Lumigator and the changes are quite simple, I am closing this for now to avoid clutter

@daavoo daavoo closed this Nov 20, 2024
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.

2 participants