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

Record decisions for update to the repository structure ahead of v5 #24

Merged
merged 1 commit into from
May 9, 2023

Conversation

romaricpascal
Copy link
Member

Prompted by alphagov/govuk-frontend#3491 and alphagov/govuk-frontend#3498, we've had discussions to clarify which updates to our repository organisation were necessary ahead of v5. This PR adds the recording of the decisions we came to.


### Removing built package files from versioning

- We could also have built the package to a separate folder that's not versioned (for ex. `govuk-frontend-local-build`) to ensure we consume the built files without impacting `package`.
Copy link
Member Author

Choose a reason for hiding this comment

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

@colinrotherham @36degrees Do any of you remember why we left that one aside or was it just to avoid the clunkyness of building the same thing in two places with slightly different names?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah by replacing the real govuk-frontend in the workspace with the fake one we'd lose:

  1. Dependency resolution testing
  2. Package exports testing
  3. Dependabot updates
  4. ESLint import/export/publishing checks

Plus we'd end up replicating things in two package.json files

Probably more

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update to try and reflect that 😄

Suggested change
- We could also have built the package to a separate folder that's not versioned (for ex. `govuk-frontend-local-build`) to ensure we consume the built files without impacting `package`.
- We could also have built the package to a separate folder that's not versioned (for ex. `govuk-frontend-local-build`) to ensure we consume the built files without impacting `package`. We'd have to duplicate efforts to maintain two `package.json` files and lose testing our dependency resolution, package exports, as well as complicate the linting and Dependabot updates.

@colinrotherham How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

The contents is good, but it's under the Removing built package… heading

The suggestion for govuk-frontend-local-build was the opposite, keeping the built package

Not a blocker for me, but maybe its own heading as another alternative we discussed

Copy link
Member Author

Choose a reason for hiding this comment

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

I was hoping the heading being under ## Alternatives considered would clarify that it's the alternatives of Removing built package that are listed. It might be that Regrouping source and built package... only having one alternative makes things a little confusing.

Copy link
Member

@36degrees 36degrees 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 writing this up so comprehensively @romaricpascal 🖊️

Have added a few slightly nitpicky comments with suggestions to try and improve clarity, but this is looking great.

decision-records/005-repository-organisation-for-v5.md Outdated Show resolved Hide resolved
decision-records/005-repository-organisation-for-v5.md Outdated Show resolved Hide resolved
decision-records/005-repository-organisation-for-v5.md Outdated Show resolved Hide resolved
decision-records/005-repository-organisation-for-v5.md Outdated Show resolved Hide resolved
decision-records/005-repository-organisation-for-v5.md Outdated Show resolved Hide resolved
decision-records/005-repository-organisation-for-v5.md Outdated Show resolved Hide resolved
decision-records/005-repository-organisation-for-v5.md Outdated Show resolved Hide resolved
@romaricpascal romaricpascal changed the title Record decisions for update to the repository organisation ahead of v5 Record decisions for update to the repository structure ahead of v5 Apr 24, 2023
@romaricpascal
Copy link
Member Author

I think that last commit adressed all the remarks (besides the one about the heading within Alternatives considered, not quite sure what to do to clarify. I think the issue is with this decision containing multiple smaller decisions rather than be a unique one).

@@ -0,0 +1,122 @@
# Decision Record Title

Update our repository structure to prepare v5

Choose a reason for hiding this comment

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

Suggested change
Update our repository structure to prepare v5
Update our repository structure to prepare for v5

@36degrees
Copy link
Member

36degrees commented May 4, 2023

As discussed in dev catch up, the repo structure has diverged a little from what was in our initial decision record, so this needs updating with the latest changes.

Adding this to the board so we remember to come back and merge this!

@domoscargin domoscargin self-assigned this May 4, 2023
@romaricpascal
Copy link
Member Author

romaricpascal commented May 9, 2023

Looks good to go for me, thanks for the edits (but I can't approve as I'm the creator of the PR) 😊

@36degrees 36degrees force-pushed the repository-organisation-v5 branch from ecb6ea0 to caf2c55 Compare May 9, 2023 16:48
@36degrees 36degrees merged commit 4162b61 into main May 9, 2023
@domoscargin domoscargin removed their assignment May 9, 2023
@36degrees 36degrees deleted the repository-organisation-v5 branch July 4, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants