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

DO NOT MERGE: Remove all conditional logic around App Management being disabled #5004

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Dec 1, 2024

WHY are these changes introduced?

Completes https://github.com/Shopify/develop-app-inner-loop/issues/2343

Removes the App Management feature flag and makes it a default part of the CLI's functionality. This should be merged when App Management is ready to be rolled out to all eligible organizations.

WHAT is this pull request doing?

  • Removes the USE_APP_MANAGEMENT_API environment variable and related checks
  • Removes any behavior implementing a special case where App Management is disabled

How to test your changes?

  1. Deploy an app using shopify app deploy
  2. Verify organizations are fetched from both Partners and App Management APIs
  3. Test app creation and management workflows to ensure they work without the feature flag

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

amcaplan commented Dec 1, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@amcaplan amcaplan changed the title Remove all conditional logic around App Management being disabled DO NOT MERGE: Remove all conditional logic around App Management being disabled Dec 1, 2024
@amcaplan amcaplan marked this pull request as ready for review December 1, 2024 22:52
@amcaplan amcaplan requested a review from a team as a code owner December 1, 2024 22:52
Copy link
Contributor

github-actions bot commented Dec 1, 2024

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@amcaplan amcaplan marked this pull request as draft December 1, 2024 22:53
Base automatically changed from centralize-app-management-gating to main December 2, 2024 11:41
@amcaplan amcaplan force-pushed the 12-02-remove_all_conditional_logic_around_app_management_being_disabled branch 2 times, most recently from 5215a55 to ec98d8c Compare December 2, 2024 12:20
Copy link
Contributor

github-actions bot commented Jan 2, 2025

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

@amcaplan
Copy link
Contributor Author

amcaplan commented Jan 2, 2025

still relevant

@amcaplan amcaplan force-pushed the 12-02-remove_all_conditional_logic_around_app_management_being_disabled branch 2 times, most recently from 917ebf1 to 81cc960 Compare January 2, 2025 15:40
@amcaplan amcaplan force-pushed the 12-02-remove_all_conditional_logic_around_app_management_being_disabled branch from 81cc960 to 9ecb7d4 Compare January 2, 2025 15:51
Copy link
Contributor

github-actions bot commented Jan 2, 2025

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/private/node/constants.d.ts
@@ -31,7 +31,6 @@ export declare const environmentVariables: {
     otelURL: string;
     themeKitAccessDomain: string;
     json: string;
-    useAppManagement: string;
 };
 export declare const defaultThemeKitAccessDomain = "theme-kit-access.shopifyapps.com";
 export declare const systemEnvironmentVariables: {
packages/cli-kit/dist/private/node/api/headers.d.ts
@@ -1,7 +1,7 @@
 /// <reference types="node" resolution-mode="require"/>
 import { ExtendableError } from '../../../public/node/error.js';
 import https from 'https';
-export declare class RequestClientError extends ExtendableError {
+declare class RequestClientError extends ExtendableError {
     statusCode: number;
     constructor(message: string, statusCode: number);
 }
@@ -26,4 +26,5 @@ export declare function buildHeaders(token?: string): {
  * if the service is running in a Spin environment, the attribute "rejectUnauthorized" is
  * set to false
  */
-export declare function httpsAgent(): Promise<https.Agent>;
\ No newline at end of file
+export declare function httpsAgent(): Promise<https.Agent>;
+export {};
\ No newline at end of file
packages/cli-kit/dist/private/node/session/scopes.d.ts
@@ -1,4 +1,3 @@
-/// <reference types="node" resolution-mode="require"/>
 import { API } from '../api.js';
 /**
  * Generate a flat array with all the default scopes for all the APIs plus
@@ -6,7 +5,7 @@ import { API } from '../api.js';
  * @param extraScopes - custom user-defined scopes
  * @returns Array of scopes
  */
-export declare function allDefaultScopes(extraScopes?: string[], systemEnvironment?: NodeJS.ProcessEnv): string[];
+export declare function allDefaultScopes(extraScopes?: string[]): string[];
 /**
  * Generate a flat array with the default scopes for the given API plus
  * any custom scope defined by the user
@@ -14,4 +13,4 @@ export declare function allDefaultScopes(extraScopes?: string[], systemEnvironme
  * @param extraScopes - custom user-defined scopes
  * @returns Array of scopes
  */
-export declare function apiScopes(api: API, extraScopes?: string[], systemEnvironment?: NodeJS.ProcessEnv): string[];
\ No newline at end of file
+export declare function apiScopes(api: API, extraScopes?: string[]): string[];
\ No newline at end of file
packages/cli-kit/dist/public/node/context/local.d.ts
@@ -26,13 +26,6 @@ export declare function isDevelopment(env?: NodeJS.ProcessEnv): boolean;
  * @returns True if SHOPIFY_FLAG_VERBOSE is truthy or the flag --verbose has been passed.
  */
 export declare function isVerbose(env?: NodeJS.ProcessEnv): boolean;
-/**
- * It returns true if the App Management API is available.
- *
- * @param env - The environment variables from the environment of the current process.
- * @returns True if the App Management API is available.
- */
-export declare function isAppManagementEnabled(env?: NodeJS.ProcessEnv): boolean;
 /**
  * Returns true if the environment in which the CLI is running is either
  * a local environment (where dev is present) or a cloud environment (spin).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant