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

ADR: use separate repo #43

Merged
merged 1 commit into from
Jun 5, 2024
Merged

ADR: use separate repo #43

merged 1 commit into from
Jun 5, 2024

Conversation

lucyb
Copy link
Contributor

@lucyb lucyb commented Jun 4, 2024

This documents our use of a separate repo for the Dockerfile and other supporting files needed for the research-template dev container configuration.

Closes opensafely-core/codespaces-initiative#63

Copy link
Member

@iaindillingham iaindillingham left a comment

Choose a reason for hiding this comment

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

As this is the project's -- and my! -- first ADR, I've left several comments. I'd be happy to talk more about them; rework the PR; do something else 🙂

docs/adr/0001-create-research-template-docker-repo.md Outdated Show resolved Hide resolved
docs/adr/0001-create-research-template-docker-repo.md Outdated Show resolved Hide resolved
Comment on lines 17 to 19
* The Dockerfile used to generate the dev containers Docker image
* The tooling and GitHub actions to build the Docker image and to run the tests
* Some of the supporting files for the research-template (e.g documentation and ADRs).
Copy link
Member

Choose a reason for hiding this comment

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

But why? For example:

We will store the Dockerfile in research-template-docker because we do not want researchers to modify it and because we want to version it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to address that in the consequences section, which I've reworded in my most recent commit. I've tried to keep the decision section short and to just state the decision without the reasoning.

Copy link
Member

Choose a reason for hiding this comment

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

Nygard doesn't mention that the decision section should describe (what) but not explain (why). If this is a principle that we're going to adopt (and I'm ambivalent 🙂), then we should add a note to docs/adr/0000-record-architecture-decisions.md.

docs/adr/0001-create-research-template-docker-repo.md Outdated Show resolved Hide resolved
Copy link
Member

@iaindillingham iaindillingham left a comment

Choose a reason for hiding this comment

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

Thanks for responding to my comments, @lucyb. You'll notice that I've made some more comments, but none are critical.

I think that a major limitation of not recording architectural decisions when those architectural decisions are made is that the context and consequences are hard to establish: we have to reconstruct the context from memory and the consequences are clearer, as we've lived with them.

I think that there's more to say about ADRs in general, but this isn't the place. Having a record is better than not; and having a record that emphasises the advantages and disadvantages of an architectural decision is better than having a record that emphasises only the advantages.

This documents our use of a separate repo for the Dockerfile and other
supporting files needed for the research-template dev container configuration.

Fixes #63
@lucyb lucyb force-pushed the adr-separate-repo branch from d0a1006 to 2fece1c Compare June 5, 2024 12:30
@lucyb
Copy link
Contributor Author

lucyb commented Jun 5, 2024

Thanks again for the feedback Iain, I think the ADR is looking much better as a result. I also agree about writing up the ADRs closer to the point the decision is made in future!

@lucyb lucyb merged commit 9347460 into main Jun 5, 2024
3 checks passed
@lucyb lucyb deleted the adr-separate-repo branch June 5, 2024 12:55
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.

ADR: Using the separate research-template-docker repo
2 participants