Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Major Bug fixes #162

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Major Bug fixes #162

wants to merge 3 commits into from

Conversation

sladyn98
Copy link
Contributor

a) Changes the docker build to false: Prevents the docker in docker condition.
b) Changes the empty editor to now set the code.
c) Change title in index.html

@sladyn98 sladyn98 added the bug Something isn't working label Aug 29, 2020
@sladyn98 sladyn98 requested a review from a team as a code owner August 29, 2020 05:53
Copy link
Contributor

@martinda martinda left a comment

Choose a reason for hiding this comment

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

When fixing bugs, please follow these general rules:

  1. present one commit per bug fix
  2. say in the commit message what bug is being fixed

This will make it much easier to review.

frontend/public/index.html Outdated Show resolved Hide resolved
@@ -70,7 +70,7 @@ private static JSONObject generateBuildSettings(final JSONObject buildSettings)
dockerInfo.put("base", buildSettings.getString("base"));
if (buildSettings.has("tag")) {
dockerInfo.put("tag", buildSettings.getString("tag"));
dockerInfo.put("build", "true");
dockerInfo.put("build", "false");
Copy link
Contributor

Choose a reason for hiding this comment

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

Dang, this sees like a pretty big change.... There should be a test that accompanies this to show that the problem existed and that the problem was solved by this particular change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This actually fails the testing at this time because the test expects "true". Setting docker to true actually fails, see #155 and this comment in #141.

Co-authored-by: Kristin Whetstone <[email protected]>
@kwhetstone
Copy link
Contributor

The test is still expecting false?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants