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

add basic fake implementation of sdaccel-builder for testing #266

Merged
merged 3 commits into from
Nov 13, 2018

Conversation

CampGareth
Copy link
Contributor

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Description

Review Checklist

Goals

  • Does it solve the problem?

  • Is it the simplest implementation of that solution?
    Does it yak shave? Does it introduce new dependencies that aren't necessary?

  • Does it decrease modularity?
    Does the user of a module need to import another module to use this one?
    If we want to delete these changes, how easy is that?

  • Does it clarify our domain?
    What things does it refine? What things get added? How does this pave the way for new things?
    Are things named in such a way that a domain expert can find them?

  • Does it introduce non-domain concepts?
    What does the user of this need to learn outside of our domain in order to use this?

Testing

  • Do we integration test changes to external services?

  • Do we unit test code we can change?

@pwaller
Copy link
Contributor

pwaller commented Sep 10, 2018

We have a huge number of top level directories - can it live under one of scripts, ci, build or somewhere else?

@CampGareth
Copy link
Contributor Author

It can indeed, making the change

@pwaller
Copy link
Contributor

pwaller commented Sep 10, 2018

Another couple of issues:

  • It's unclear from the PR how this is actually used (who builds it? what causes testing to use it?)
  • From talking to Max in person, it is meant to be tagged in the docker registry with the production build URL so that the platform picks it up when running the job under fake batch. This is inappropriate, because it comes with an associated risk of pushing the test image to production.

I suggest inlining this PR into the PR which starts using it, and fixing the latter issue by having something during the tests which builds the image (with a test image name) and something in the test which causes it to pick up this test image whilst running in development (as opposed to prod).

@pwaller pwaller force-pushed the feature/fake-sdaccel-builder branch from 688c0c8 to eb2bf87 Compare September 10, 2018 10:23
@CampGareth
Copy link
Contributor Author

I don't think there's a clear need for this for now. I'll re-open it if one appears in future.

@CampGareth CampGareth closed this Nov 12, 2018
@CampGareth CampGareth deleted the feature/fake-sdaccel-builder branch November 12, 2018 14:06
@CampGareth CampGareth restored the feature/fake-sdaccel-builder branch November 13, 2018 16:35
@CampGareth CampGareth reopened this Nov 13, 2018
@CampGareth
Copy link
Contributor Author

Re-opening this in the wake of #303

I think if this ends up as an image being created by docker-compose up and then gets tagged as "fake-sdaccel-builder" I think this is useful as it'll be picked up by fake-batch automatically per a commit that I've... made in another branch. Hmm. Some organisational work to be done here.

@CampGareth CampGareth changed the base branch from master to feature/on-prem-fake-sdaccel November 13, 2018 16:58
@CampGareth CampGareth merged commit eb6f968 into feature/on-prem-fake-sdaccel Nov 13, 2018
@CampGareth CampGareth deleted the feature/fake-sdaccel-builder branch November 13, 2018 17:01
CampGareth pushed a commit that referenced this pull request Nov 14, 2018
* build fake-sdaccel-builder image for fake-batch testing
CampGareth pushed a commit that referenced this pull request Nov 28, 2018
* build fake-sdaccel-builder image for fake-batch testing
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.

2 participants