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: docker-compose point to master tag #27179

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

dpgaspar
Copy link
Member

SUMMARY

We now use master and master-dev for latest master tags. This proposes a small change that will probably help people here: #26997 combined with #27146

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@@ -14,7 +14,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
x-superset-image: &superset-image apachesuperset.docker.scarf.sh/apache/superset:${TAG:-latest}
x-superset-image: &superset-image apachesuperset.docker.scarf.sh/apache/superset:${TAG:-master}
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to have devs work from master, but for production, wouldn't it be better for them to work from an official release tag?

Copy link
Member Author

Choose a reason for hiding this comment

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

prob, if that's what's expected

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with @eschutho - I don't understand all of #26997 but it seems like these people are all trying to run latest builds off master instead of an official release? Or using a mismatch of master repo clone + an official release. Which underscores for me the value of directing new users to an official release instead of master, so that they have a positive experience right out of the box.

Copy link
Member

Choose a reason for hiding this comment

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

See also my comment here: #26997 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, just checked, and docker-compose-non-dev does not mount the current code so latest is probably the best tag option here.

@dpgaspar dpgaspar merged commit 7330125 into apache:master Feb 22, 2024
27 checks passed
@dpgaspar dpgaspar deleted the fix/docker-compose branch February 22, 2024 08:50
@mistercrunch
Copy link
Member

Awesome. I just merged #27146 which should make the master builds to also be multi-platform, meaning this should work and it will support Apple silicon.

But note that there's something loose here, where it looks like the assumption with docker-compose is that you can mount the code on your local master with the latest master image build and things should just work. That's not super solid/guaranteed. You could be on an older commit that say assumes a python req that's been removed on the latest master. Or you could be ahead of master, introduce a new python req, and for it now be reflected on the image.

Knowing this, should we set docker-compose to "docker build" from the Dockerfile and simply leverage the caching?

@dpgaspar
Copy link
Member Author

Awesome. I just merged #27146 which should make the master builds to also be multi-platform, meaning this should work and it will support Apple silicon.

But note that there's something loose here, where it looks like the assumption with docker-compose is that you can mount the code on your local master with the latest master image build and things should just work. That's not super solid/guaranteed. You could be on an older commit that say assumes a python req that's been removed on the latest master. Or you could be ahead of master, introduce a new python req, and for it now be reflected on the image.

Knowing this, should we set docker-compose to "docker build" from the Dockerfile and simply leverage the caching?

that's a valid point, I guess it depends on how much time it would normally take to build vs the frequency of master requirements being off.

How can we leverage caching when docker-compose build points to Dockerfile?
There must be a way

@mistercrunch
Copy link
Member

How can we leverage caching when docker-compose build points to Dockerfile?

I think it should leverage cached layers if pointed to the right cache that supports the master builds (apache/superset-cache). In theory a build that doesn't touch the dependencies should be really fast. I proposed something on #26997 (comment)

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.1.0 labels Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 4.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants