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

remove files that are needed to run airbyte-platform #25323

Merged
merged 5 commits into from
Apr 24, 2023

Conversation

c-p-b
Copy link
Contributor

@c-p-b c-p-b commented Apr 19, 2023

What

These were already gitignored, but the actual deletion of the filesmust have slipped through the cracks as part of the monorepo migration. They are currently causing confusion for end users as the versions are mismatching with what is currently in airbyte-platform.

All of these files are downloaded and updated on-the-fly when running "run-ab-platform.sh". So they should be removed from here and gitignored from this repository

How

Delete the files

@c-p-b c-p-b requested a review from timroes April 19, 2023 16:14
.env Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will actually colide with the PR Joey just did: #25311 since we're reading the VERSION from this file at the moment to figure out which OSS release to load. This also seems to be correctly bumped still in the release process. Should we write that in some other specific file and adjust the run platform script again, or keep just a VERSION around in this file, or some fully other solution?

Copy link
Contributor Author

@c-p-b c-p-b Apr 19, 2023

Choose a reason for hiding this comment

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

I think we should create something like an extensionless VERSION file that has this format, and only has one line in it that has version line identically as it looks in the .env (to save work changing things downstream). Maybe call it PLATFORM_VERSION or something. I would rather leave .env gitignored so that it's super clear that it's identical and coming from the platform.

Any particular feelings about the name or file format for this hypothetical one line file?

Copy link
Contributor Author

@c-p-b c-p-b Apr 19, 2023

Choose a reason for hiding this comment

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

Another, kind of cleaner possibly solution is to just store that version in run-ab-platform.sh and have bumpversion.cfg hit that instead. Then it's SUPER clear that it refers to the platform version and keeps everything in one file

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel PLATFORM_VERSION would be a nice name and being clear enough, alternatively we could call it CURRENT_PLATFORM_VERSION, but at some point this name is just getting too long :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The canonical version now lives in run-ab-platform.sh after this change (in order to make downloading and running airbyte just wget a single file instead of having to clone entire airbyte repo)

@c-p-b
Copy link
Contributor Author

c-p-b commented Apr 20, 2023

internal pr to bump run-ab-platform.sh as part of the OSS release process: https://github.com/airbytehq/airbyte-platform-internal/pull/6056

@c-p-b c-p-b requested a review from josephkmh April 20, 2023 17:58
@@ -16,6 +16,8 @@ serialize =

[bumpversion:file:octavia-cli/README.md]

[bumpversion:file:run-ab-platform.sh]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why, but putting this at the bottom caused bumpversion to fail. It only worked when I put it here before the setup.py line

@@ -1,5 +1,6 @@
#!/bin/bash

VERSION=0.44.1
Copy link
Contributor

@josephkmh josephkmh Apr 21, 2023

Choose a reason for hiding this comment

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

I think we want this injected down on line 44 too:

base_github_url="https://raw.githubusercontent.com/airbytehq/airbyte-platform/v$VERSION/"

Otherwise this script will download docker-compose.yaml, .env and co from main rather than from the latest release tag. main can contain breaking changes in docker-compose.yaml (as a recent PR of mine did) and then people were running the new docker-compose.yaml with "old" images from the most recent release, which caused problems.

You probably want to rebase on master first, since I touched that line in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and done!

@c-p-b c-p-b merged commit 08f83b8 into master Apr 24, 2023
@c-p-b c-p-b deleted the remove-ab-platform-files branch April 24, 2023 13:14
marcosmarxm pushed a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
* remove files that are needed to run airbyte-platform

* make run-ab-platform.sh the canonical version location

* remove version bump used for testing

* pull files from correct version of platform
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