Skip to content

Option to install gems for only target ecosystem in dev shell#5520

Closed
mctofu wants to merge 2 commits intomainfrom
mctofu/dev-shell-target-ecosystem
Closed

Option to install gems for only target ecosystem in dev shell#5520
mctofu wants to merge 2 commits intomainfrom
mctofu/dev-shell-target-ecosystem

Conversation

@mctofu
Copy link
Copy Markdown
Contributor

@mctofu mctofu commented Aug 11, 2022

This speeds up build time of the dev shell if you only need to work with a particular ecosystem. Installing gems for all ecosystems takes a long time so this flag allows you to only install them for a particular ecosystem. You'll be able to dry-run/test the target ecosystem but you'll need to rebuild or manually install gems to work with others.

TARGET_ECOSYSTEM=go_modules ./bin/docker-dev-shell --rebuild

My bash skills are a bit rusty so open to any tips on improving this as well!

This speeds up build time of the dev shell if you only need to
work with a particular ecosystem.
@mctofu mctofu requested a review from a team as a code owner August 11, 2022 23:19
Copy link
Copy Markdown
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

It's neat and we should merge it, but one nit is that it's not really discoverable for new users of the dev shell.

Since this isn't the dry-run script, I'm not exactly sure how to make this discoverable... if we should be printing potential args at the beginning fo the script run, IDK... and potentially part of this may be replaced (at least partially) by the upcoming work of splitting out the docker images (at least if we end up going down that route) so right now it's not worth investing additional time into docs/discoverability... but something to keep in mind on whenever we add more dev options

COPY --chown=dependabot:dependabot python/Gemfile python/dependabot-python.gemspec ${CODE_DIR}/python/
COPY --chown=dependabot:dependabot pub/Gemfile pub/dependabot-pub.gemspec ${CODE_DIR}/pub/
COPY --chown=dependabot:dependabot terraform/Gemfile terraform/dependabot-terraform.gemspec ${CODE_DIR}/terraform/
ARG TARGET_ECOSYSTEM=
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add a code comment right above that says "If specified, will only build that ecosystem. Blank will build for all ecosystems. Speeds up any debugging that requires recompiling an ecosystem".

I realize that increase the risk of the comment going out of sync with the code, but in this case it's worth it IMO simply for discoverability if someone is skimming the source it's super easy to miss that this flag exists.

Copy link
Copy Markdown
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

Nice one! Maybe stick a note in the README?

Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

I run into this just yesterday and it was indeed painfully slow! So thanks!! :)

Maybe a silly question, but I see that all these Gemfiles only install the dependencies in common/. Would it be a bad idea to share this gem location accross ecosystems? Just saying because that would make this fast by default.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

This is what I was thinking: #5528. Not sure if it works but makes sense to me in principle?

@mctofu
Copy link
Copy Markdown
Contributor Author

mctofu commented Aug 12, 2022

Maybe a silly question, but I see that all these Gemfiles only install the dependencies in common/. Would it be a bad idea to share this gem location accross ecosystems? Just saying because that would make this fast by default.

Oh interesting, I didn't realize the gems aren't customized for any of the ecosystems! I think sharing them would make more sense although it would make it harder to add a gem to a single ecosystem in the future. That may not be a problem given it hasn't happened yet.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Yeah!

It that happened, I think it might still work, just the other environments would have one unused gem available. But we could split out that single bundle anyways (partially rolling this back indeed, so with some extra work).

Anyways, I think we can give #5528 a try and see if it has any unexpected side effects!

@jurre
Copy link
Copy Markdown
Member

jurre commented Aug 12, 2022

FYI: there are some places where we do install dependencies outside of the common gemspec, not sure if that matters in this context

spec.add_dependency "parseconfig", "~> 1.0", "< 1.1.0"

We could also move it into common if it's painful otherwise?

@mctofu
Copy link
Copy Markdown
Contributor Author

mctofu commented Aug 13, 2022

FYI: there are some places where we do install dependencies outside of the common gemspec, not sure if that matters in this context

Aha, nice find! I think that means the submodule updater would be missing that gem with #5528? If that's the only case I wouldn't be opposed to moving it to common as that makes the dev containers faster to build/easier to use for everyone.

Does it make sense to install the gems for the omnibus first since that should include dependencies from all the sub-projects? ex: #5529

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

I verified that with #5528 the submodule updater is indeed missing the parseconfig gem, nice catch @jurre! So I'm going to close this in favor of #5529!

@deivid-rodriguez deivid-rodriguez deleted the mctofu/dev-shell-target-ecosystem branch August 15, 2022 15:43
@deivid-rodriguez
Copy link
Copy Markdown
Contributor

I actually meant to close my own PR, not this one, sorry 😳.

But I guess #5529 looks promising enough to close this one too. We can reopen if we end up not merging #5529, sounds good?

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.

4 participants