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

Fix #371 #372

Merged
merged 10 commits into from
Feb 15, 2019
Merged

Fix #371 #372

merged 10 commits into from
Feb 15, 2019

Conversation

LazarusX
Copy link
Contributor

No description provided.

Copy link
Member

@jongio jongio left a comment

Choose a reason for hiding this comment

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

Ideally, tests should be added to cover this scenario.

@@ -145,7 +145,7 @@ def build_push(self, template_file, default_platform, no_build=False, no_push=Fa
# get the Dockerfile from module.json
dockerfile = module.get_dockerfile_by_platform(platform)
container_tag = "" if self.envvars.CONTAINER_TAG == "" else "-" + self.envvars.CONTAINER_TAG
tag = "{0}:{1}{2}-{3}".format(module.repository, module.tag_version, container_tag, platform).lower()
tag = "{0}:{1}{2}-{3}".format(module.repository, module.tag_version, container_tag, platform)
Copy link
Member

Choose a reason for hiding this comment

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

FYI for future reviewers...this comment can be closed.

Image name needs to be lower case. module.repository = {server}/{name}, so you need to call .lower on that, but not on the tag. I have confirmed that name.lower is called earlier in the code.

- powershell: |
choco install jdk11
$env:JAVA_HOME = "C:\Program Files\Java\jdk-11.0.1"
choco install jdk8 --version 8.0.201
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switch to JDK8 because it's already specified as a dependency of maven. Installing JDK11 will cost more time and space.

@LazarusX
Copy link
Contributor Author

Currently CI build will fail on Windows agents only because Windows agents are still based on 1803 while the scaffolded Dockerfiles are based on 1809 (Azure DevOps' 1809 support is coming by end of Feb).

setup.py Outdated
@@ -32,9 +32,10 @@ def run(self):
'Click>=6.0',
'docker>=3.5',
'python-dotenv',
'requests',
'requests<2.21',
Copy link

Choose a reason for hiding this comment

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

what is the issue if use higher version?

Copy link

Choose a reason for hiding this comment

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

And can we make sure what is the lowest version supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lower bound specified.

setup.py Outdated
'fstrings',
'msrestazure~=0.4.32',
'azure-cli-core<2.0.55',
Copy link

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

azure-cli-core depends on a beta version of PyYAML starting version 2.0.55, which will conflict with docker-compose. See #373

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lower bound specified.

@LazarusX LazarusX merged commit 81e7655 into master Feb 15, 2019
@LazarusX LazarusX deleted the ray-371 branch February 15, 2019 07:37
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.

3 participants