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.

Sign up for GitHub

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

Jump to bottom

fix: fix docker build #4461

Merged
merged 13 commits into from
Oct 22, 2020
Merged

fix: fix docker build #4461

merged 13 commits into from
Oct 22, 2020

Conversation

a-b-r-o-w-n
Copy link
Contributor

Description

Updates docker build after moving extensions directory outside ./Composer. Also enables docker builds in CI again.

Task Item

fixes #4435

@coveralls
Copy link

coveralls commented Oct 21, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.005%) to 55.358% when pulling 5a95fd7 on abrown/fix-compile-extensions into d18b0fb on main.

@a-b-r-o-w-n
remove buildx
b1e28b6
@a-b-r-o-w-n
force extensions to install all dependencies
8d3967c 
when NODE_ENV is production, yarn will not install dev dependencies. We need those to build extensions.
@a-b-r-o-w-n
revert not using buildx
d71b48e
@a-b-r-o-w-n
Copy link
Contributor Author

@cdonke Can you take a look at this PR and let me know if there are any optimizations that can be made?

@cdonke
Copy link
Contributor

cdonke commented Oct 21, 2020

@a-b-r-o-w-n Sure! right now!

@cdonke
Copy link
Contributor

cdonke commented Oct 21, 2020

@a-b-r-o-w-n

Some dependencey requires the .git folder
image

Not sure if it is strictly necessary. If it is, I would COPY the .git folder as well, just after line 16
COPY /.git ../.git

I saw that you added a try-catch block on /Composer/scripts/compileExtensions.js because of the git command it uses. It is complaining that the git command is unknown. You have 2 options:

  1. remove the whole getLastModified method, because it will always fail,
  2. or to add Git to the container, adding this just after line 10:
    RUN apk add --no-cache git

@a-b-r-o-w-n
Copy link
Contributor Author

@cdonke thanks for the review. The dependency that is complaining about the .git directory is when developing locally. It runs eslint before each commit. It's fine to ignore in the docker environment.

As for the try/catch in compileExtensions.js, in the case of not having git installed, I will treat that as always needing to be compiled. I don't think the docker container needs git installed because we should always build the extensions.

@cdonke
Copy link
Contributor

cdonke commented Oct 21, 2020

@a-b-r-o-w-n Makes sense...

So I don't see any more changes... besides removing a redundant WORKDIR at line 35... but that is not affecting anything...

@a-b-r-o-w-n a-b-r-o-w-n merged commit e8d0d55 into main Oct 22, 2020
@a-b-r-o-w-n a-b-r-o-w-n deleted the abrown/fix-compile-extensions branch October 22, 2020 00:03
@cwhitten cwhitten mentioned this pull request Nov 13, 2020
Merged
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
@a-b-r-o-w-n
fix: fix docker build (microsoft#4461)
d2fabd8 
* always compile extensions if git not present

* compile extension if main module is missing

* do not print git errors

* fix docker builds

* re-enable docker on CI

* load image into docker after building

* report on errors and fail script

* remove buildx

* force extensions to install all dependencies

when NODE_ENV is production, yarn will not install dev dependencies. We need those to build extensions.

* revert not using buildx

* start up docker in background

* update code scan action to run on push to main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beyackle beyackle Awaiting requested review from beyackle

@boydc2014 boydc2014 Awaiting requested review from boydc2014

@cwhitten cwhitten Awaiting requested review from cwhitten

@GeoffCoxMSFT GeoffCoxMSFT Awaiting requested review from GeoffCoxMSFT

@hatpick hatpick Awaiting requested review from hatpick

@srinaath srinaath Awaiting requested review from srinaath

@tonyanziano tonyanziano Awaiting requested review from tonyanziano

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Docker not building
3 participants
@a-b-r-o-w-n @coveralls @cdonke