Re: [PR] fix version check [airflow]
jscheffl merged PR #61318: URL: https://github.com/apache/airflow/pull/61318 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix version check [airflow]
Ajay9704 commented on PR #61318: URL: https://github.com/apache/airflow/pull/61318#issuecomment-3884196586 @jscheffl I believe I've implemented all the requested changes, and all checks are now passing. Whenever you get a chance, please review and let me know if there’s anything else I may have missed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix version check [airflow]
Ajay9704 commented on PR #61318: URL: https://github.com/apache/airflow/pull/61318#issuecomment-3883046014 @jscheffl as you suggested i have made all this changes . Extracted getLegacyRouterNavigation() into versionUtils.ts to handle pre-release versions (e.g., 3.1.7rc1) correctly Moved tests to utils/versionUtils.test.ts to test the extracted function Removed redundant NavTabs.test.ts which was testing the semver library Reverted unnecessary JSX formatting in NavTabs.tsx to match original Updated exports in utils/index.ts Rebuilt assets and updated www-hash.txt Please review when convenient. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix version check [airflow]
jscheffl commented on code in PR #61318:
URL: https://github.com/apache/airflow/pull/61318#discussion_r2789979511
##
providers/edge3/src/airflow/providers/edge3/plugins/www/src/layouts/NavTabs.tsx:
##
@@ -33,46 +33,57 @@ export const NavTabs = ({ tabs }: Props) => {
const containerRef = useRef(null);
const containerWidth = useContainerWidth(containerRef);
- const { data } = useQuery<{version: string, git_version: string | null}>({
+ const { data } = useQuery<{ version: string; git_version: string | null }>({
queryFn: async () => {
const res = await axios.get("/api/v2/version");
return res.data;
},
queryKey: ["appVersion"],
});
- let legacyRouterNavigation: boolean | undefined = undefined;
+ let legacyRouterNavigation: boolean | undefined;
if (data) {
-const airflowCoreVersion = data.version;
-if (lte(airflowCoreVersion, "3.1.6")) {
- legacyRouterNavigation = true;
-} else {
- legacyRouterNavigation = false;
+// Normalize Airflow version using semver.coerce to handle pre-release
suffixes
+// coerce() converts "3.1.7rc1" to "3.1.7" for proper version comparison
+const coercedVersion = coerce(data.version);
+const airflowCoreVersion = coercedVersion?.version ?? null;
+
+if (airflowCoreVersion) {
+ // Legacy navigation for versions <= 3.1.6
+ legacyRouterNavigation = lte(airflowCoreVersion, "3.1.6");
}
}
return (
- {legacyRouterNavigation !== undefined ? tabs.map(({ icon, label, value
}) => (
Review Comment:
I do not understand why this code block - which obviously has no changes(?)
is re-formatted. Can you revert this? So that changes are only up to line 54?
##
providers/edge3/src/airflow/providers/edge3/plugins/www/src/layouts/NavTabs.test.ts:
##
Review Comment:
Thanks for adding the test. Actually what you test here is the version
library and not the code/widget.
Can you either remove the test (and we assume the library is tested
upstream) or extract the version check in NavTabs.tsx into a library/function
and then use the test to test the function for the cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix version check [airflow]
Ajay9704 commented on code in PR #61318:
URL: https://github.com/apache/airflow/pull/61318#discussion_r2787988302
##
providers/edge3/src/airflow/providers/edge3/plugins/www/src/layouts/NavTabs.test.ts:
##
@@ -0,0 +1,97 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { coerce, lte } from "semver";
+import { describe, expect, it } from "vitest";
+
+/**
+ * Tests for version parsing logic used in NavTabs.tsx
+ * Ensures pre-release versions like "3.1.7rc1" are correctly normalized
+ */
+describe("Version Parsing with semver.coerce()", () => {
+const parseVersion = (version: string): string | null => {
+const coercedVersion = coerce(version);
+return coercedVersion?.version ?? null;
+};
+
+it("handles standard semantic versions", () => {
+expect(parseVersion("3.1.7")).toBe("3.1.7");
+expect(parseVersion("3.1.6")).toBe("3.1.6");
+expect(parseVersion("3.0.0")).toBe("3.0.0");
+});
+
+it("handles release candidate versions (e.g., 3.1.7rc1)", () => {
+expect(parseVersion("3.1.7rc1")).toBe("3.1.7");
+expect(parseVersion("3.1.7rc2")).toBe("3.1.7");
+expect(parseVersion("3.2.0rc1")).toBe("3.2.0");
+});
+
+it("handles pre-release versions with dash separator", () => {
Review Comment:
changes made accordingly , please review sir
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix version check [airflow]
Ajay9704 commented on PR #61318: URL: https://github.com/apache/airflow/pull/61318#issuecomment-3877407104 @jscheffl I know this took some time, but I wanted to make sure everything was done properly. I went through the docs , set up a proper Linux environment, fixed my local setup . All checks are now passing successfully. Please let me know if anything else is needed from my side , happy to address it . -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix version check [airflow]
jscheffl commented on PR #61318: URL: https://github.com/apache/airflow/pull/61318#issuecomment-3835876661 > > (1) the package-lock.json should not be committed. You can setup your devenv as described in https://github.com/apache/airflow/blob/main/contributing-docs/03a_contributors_quick_start_beginners.rst and with prek the static assets are locally re-generated including the needed www-hash.txt > > (2) Please check the PR template banner and use the template > > (3) I assume the fix is not working by reading the code. Please check with the commands given in the issue title included if it really fixes. I do not see how a pre-release suffix shall be cut and frankly speaking the fix looks like AI slop > > Unrelated to current PR but related to the release process. I submitted another PR and noticed that the www-hash.txt that was generated on my laptop differed from the one generated by CI / CD tool. I assumed it might be related to using mac and linux on ci/cd. Does that make sense? Should not differ. But not tamper proof. Most developers work with Linux and/or MasOS. So might be you were hitting a but. But hopefully static building of assets will be removed soon, still working on it in https://github.com/apache/airflow/pull/56456 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix version check [airflow]
shivaam commented on PR #61318: URL: https://github.com/apache/airflow/pull/61318#issuecomment-3835839595 > (1) the package-lock.json should not be committed. You can setup your devenv as described in https://github.com/apache/airflow/blob/main/contributing-docs/03a_contributors_quick_start_beginners.rst and with prek the static assets are locally re-generated including the needed www-hash.txt > > (2) Please check the PR template banner and use the template > > (3) I assume the fix is not working by reading the code. Please check with the commands given in the issue title included if it really fixes. I do not see how a pre-release suffix shall be cut and frankly speaking the fix looks like AI slop Unrelated to current PR but related to the release process. I submitted another PR and noticed that the www-hash.txt that was generated on my laptop differed from the one generated by CI / CD tool. I assumed it might be related to using mac and linux on ci/cd. Does that make sense? Here is the PR I am taking about: https://github.com/apache/airflow/pull/60908 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix version check [airflow]
jscheffl commented on code in PR #61318:
URL: https://github.com/apache/airflow/pull/61318#discussion_r2753573646
##
providers/edge3/src/airflow/providers/edge3/plugins/www/src/layouts/NavTabs.test.ts:
##
@@ -0,0 +1,97 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { coerce, lte } from "semver";
+import { describe, expect, it } from "vitest";
+
+/**
+ * Tests for version parsing logic used in NavTabs.tsx
+ * Ensures pre-release versions like "3.1.7rc1" are correctly normalized
+ */
+describe("Version Parsing with semver.coerce()", () => {
+const parseVersion = (version: string): string | null => {
+const coercedVersion = coerce(version);
+return coercedVersion?.version ?? null;
+};
+
+it("handles standard semantic versions", () => {
+expect(parseVersion("3.1.7")).toBe("3.1.7");
+expect(parseVersion("3.1.6")).toBe("3.1.6");
+expect(parseVersion("3.0.0")).toBe("3.0.0");
+});
+
+it("handles release candidate versions (e.g., 3.1.7rc1)", () => {
+expect(parseVersion("3.1.7rc1")).toBe("3.1.7");
+expect(parseVersion("3.1.7rc2")).toBe("3.1.7");
+expect(parseVersion("3.2.0rc1")).toBe("3.2.0");
+});
+
+it("handles pre-release versions with dash separator", () => {
Review Comment:
We do not use versions other than "rcN", also not with dashes. So these
tests can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix version check [airflow]
jscheffl commented on PR #61318: URL: https://github.com/apache/airflow/pull/61318#issuecomment-3831101899 (1) the package-lock.json should not be committed. You can setup your devenv as described in https://github.com/apache/airflow/blob/main/contributing-docs/03a_contributors_quick_start_beginners.rst and with prek the static assets are locally re-generated including the needed www-hash.txt (2) Please check the PR template banner and use the template (3) I assume the fix is not working by reading the code. Please check with the commands given in the issue title included if it really fixes. I do not see how a pre-release suffix shall be cut and frankly speaking the fix looks like AI slop -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix version check [airflow]
Ajay9704 commented on PR #61318: URL: https://github.com/apache/airflow/pull/61318#issuecomment-3830983323 > No sorry, this is the wrong place and wrong side. > > The version check failing in #61313 is the plugin UI check that is made in Typescript. Not in the Python code. @jscheffl Sorry, earlier I was wrong. I found the actual issue now, and I think it should be fixed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
