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

psql outputs it's NOTICE messages to stderr causing the action to fail #33

Open
paw-eloquent-safe opened this issue Sep 30, 2022 · 9 comments

Comments

@paw-eloquent-safe
Copy link

paw-eloquent-safe commented Sep 30, 2022

psql outputs it's NOTICE messages to stderr causing the whole action to fail even if the exit code is 0.
The offending lines of code:

const options: any = {
listeners: {
stderr: (data: Buffer) => {
error += data.toString();
}
}
};

if (error) {
throw new Error(`error in file: ${file}\n${error}`);
}

NOTICE's should be treated as warnings not errors those would be EXCEPTION's.

Maybe there should be an option for choosing to ignore stderr and instead rely on the exit code of psql

It's trivial to get a NOTICE to fail the action.
Consider the following output generated by dotnet ef migrations script -s Proj.Bootstrap -p Proj.Data --idempotent -o migration.sql:

CREATE TABLE IF NOT EXISTS "__EFMigrationsHistory" (
    "MigrationId" character varying(150) NOT NULL,
    "ProductVersion" character varying(32) NOT NULL,
    CONSTRAINT "PK___EFMigrationsHistory" PRIMARY KEY ("MigrationId")
);

START TRANSACTION;
....
COMMIT;

If the __EFMigrationsHistory table already exists, the action fails, because psql:/migration.sql:5: NOTICE: relation "__EFMigrationsHistory" already exists, skipping gets written to stderr.

@cotzo
Copy link

cotzo commented Feb 16, 2023

any updates on this issue? this is blocking any attempt to use this action for EntityFramework migrations

@MagnusSandgren
Copy link

I am experiencing the same issue. @k-visscher / @cotzo - what did you guys do as a workaround? 🙂

@cotzo
Copy link

cotzo commented Mar 24, 2023

Nothing you can do but set "continue-on-error: true" on the action step

@MagnusSandgren
Copy link

MagnusSandgren commented Mar 24, 2023

Ah well, that's mildly terrifying 😅 I would like the workflow to fail if the migration fails. Thanks for the response @cotzo 🙂

@MagnusSandgren
Copy link

MagnusSandgren commented Mar 24, 2023

Hopefully the issue will be resolved someday, until then I've created a verbose workaround for those interested 🙂

- name: Fetch runners public IP
  id: ip
  uses: haythem/[email protected]

- name: Open postgre firewall for migrations
  run: >
    az postgres flexible-server firewall-rule create 
    --rule-name GithubActions 
    --name ${{env.PostgresServerName}}
    --resource-group ${{env.AzureResourceGroup}}
    --start-ip-address ${{ steps.ip.outputs.ipv4 }}

- name: Apply EF migration script
  run: psql -f ./migrate.sql -q ${{ secrets.PSQL_CONNECTIONSTRING }}

- name: Close postgre firewall
  if: ${{failure() || success()}} # <= Make sure to delete the firewall rule regardless of the migration result
  run: >
    az postgres flexible-server firewall-rule delete 
    --rule-name GithubActions 
    --name ${{env.PostgresServerName}}
    --resource-group ${{env.AzureResourceGroup}}
    --yes

The secrets.PSQL_CONNECTIONSTRING is formated as follows: host=MyHost.postgres.database.azure.com port=5432 dbname=postgres user=postgres password=password sslmode=require.

@paw-eloquent-safe
Copy link
Author

I am experiencing the same issue. @k-visscher / @cotzo - what did you guys do as a workaround? 🙂

I went the ontinue-on-error: true YOLO route, that @cotzo also suggested, the project was small enough that I could afford to manually check every now and then if the step actually succeeded or not.

@MagnusSandgren
Copy link

After further inspection of the code, it looks to me like the entire if block in PsqlFilesExecutor.ts can be, and should be removed. I believe this to be the case because the exec option IgnoreReturnCode is false by default and not overwritten by this repo. Meaning if the psql command returns with a return code other than 0, the exec package will create, and reject the promise with the error.

In other words - the if block will only trigger if the command writes to stderr and returns 0. However, if the command returns something other than 0, the error will flow right past the if block.

@seryckd
Copy link

seryckd commented May 17, 2023

Same problem here, NOTICES causing the action to fail. It would be preferred that only the exit code should be checked.

The workaround I am using is to disable writing notices by adding this to the top of the SQL file.

SET client_min_messages TO WARNING;

It's not ideal, and I may need to use the workaround provided by @MagnusSandgren.

@mehradn7
Copy link

Hi @BALAGA-GAYATRI @aksm-ms @microsoftopensource @N-Usha @AmrutaKawade @kanika1894, can we have a status about this issue and more generally about this repository? Is it still being maintained? There are some pending issues such as this one, and the last commit dates back to 1 year ago so it would be great to know if we should wait for a fix or consider other alternatives.

Thanks!

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

No branches or pull requests

5 participants