Ftp exist check #178

phocks · 2022-11-03T03:48:56Z

Hi all, I'm going to open this PR as it basically works as is and could be merged, but I could maybe do with some feedback about developing it more and how to handle some edge cases, also as I'm not too familiar with Yeoman and all its user interaction stuff.

So what it does is this:

When a user generates a new project with aunty new it uses Yeoman's validate feature to run a function that lists the contents of the /www/res/sites/news-projects/ FTP directory and checks the folder names against the sluggified project name. If a match is found it doesn't let the user proceed and asks them to choose a different project name.

So the edge cases are:

If a user is not on the ABC network the FTP request will obviously fail and throw an FTP error to the cli then continue (with no FTP exist check)
If a user is offline the same thing will happen
If a user doesn't have their .abc-credentials file the same thing will happen
If a user's FTP password is out of date the same thing will happen

I guess my question is, what do we do when one of these errors happens. My feeling is that maybe we just console.log a warning that the FTP check failed and that there is a small possibility that the project already exists on the server, and then just leave it to the user to check manually or pray that it doesn't exist.

Keen to hear feedback.

phocks · 2022-11-03T04:04:22Z

^example screenshot when it finds an existing directory

Also aunty init may need an ftp check (though it will be more difficult because we basically would have to tell the user to exit the generation process, change the directory name, and then try again).

ps. PR closes #105

drzax

Our conversation earlier makes a lot more sense now I've looked at this.

I had assumed the check would be at deploy time which is why I wasn't concerned that the check might fail if you weren't connected to the network (because the deploy would also fail).

I like the idea of checking at project init and this is a useful check, but I think we need check at deploy time too.

drzax · 2022-11-03T06:55:59Z

package.json

@@ -41,6 +41,7 @@
    "babel-core": "^7.0.0-bridge.0",
    "babel-jest": "^26.1.0",
    "babel-loader": "^8.2.2",
+    "basic-ftp": "^5.0.2",


I wonder if there's anything ftp-deploy does that's special. It'd be ideal if we didn't depend on two separate FTP packages.

Yeah I agree. I had to use the alternate FTP package because ftp-deploy simply deploys and doesn't have too much other functionality.

Hey @drzax I spoke to @colingourlay a bit this morning. My thinking on this is I'd like to not mess around with ftp-deploy which we've relied on in production for ages and it does its 1 job very well, as I don't know the full functionality of basic-ftp. Technically it should deploy the same, but seeing as the 1 job of this PR is to check for existing directories and not overwrite them, I don't really want to be messing around with implementing a different deploy library, which may even have unexpected consequences of overwriting other directories if not properly battle-tested with different configs and environments etc.

Happy to go to work on this feature straight away after merging this PR though. What do you think?

drzax · 2022-11-03T07:09:27Z

src/generators/project/index.js


 // Ours
 const { OUTPUT_DIRECTORY_NAME } = require('../../constants');
 const { cmd, hvy, opt, sec } = require('../../utils/color');
 const { success } = require('../../utils/logging');
 const { installDependencies } = require('../../utils/npm');
 const { combine } = require('../../utils/structures');
+const { getCredentials } = require('../../config/deploy');
+
+const DEPLOY_DIRECTORY = '/www/res/sites/news-projects/';


Deploy directory isn't necessarily a constant. It would be good if (a) this path were not defined in multiple places (it's also here) and, (b) at deploy time, it checked based on the actual deploy where contentftp is the deploy target in .aunty/deploy.json.

OK after talking with @colingourlay I'm going to get the deploy directory elsewhere and delete this.

drzax · 2022-11-03T07:21:01Z

src/config/deploy.js

@@ -139,3 +139,16 @@ module.exports.addProfileProperties = config => {

  return combine(config, profileProps);
 };
+
+module.exports.getCredentials = () => {


The config resolution is quite complex so it might be best if @colingourlay could advise, but it would be nice if credentials were fetched the same way as for deploy. For example, it seems from https://github.com/abcnews/aunty/blob/main/src/config/deploy.js#L123 that it's possible to set the profiles file path to something other than the default for the purposes of deploy and we wouldn't want to end up with a mismatch here.

Yes this would be nice. @colingourlay I might grab you to run through a few things when I can.

Same with this, should be able to get it from getDeployConfig from what @colingourlay tells me.

phocks · 2022-11-04T01:16:26Z

Thanks @drzax these are really helpful and yep @colingourlay's help on this would be great too. I agree we should probably have it check at deploy and release time and just ask the user if they're sure they want to overwrite to the directory each time. I just had concerns because after a project is created and deployed for the 1st time, then the user will have to say Y (yes) they want to overwrite on every subsequent deploy/release.

I'm just spitballing here now having a think .... what if we generated a unique ID for each project when created and put it in the base directory or something, that way we could check that the remote unique ID matches the local unique ID on deploy/upload and then we wouldn't even really need to ask them every time. Would that work?

drzax · 2022-11-04T08:58:49Z

I would want it to check if the specific branch or tag I'm deploying already exists. So for most release commands it wouldn't need to ask, but for a branch deploy it would ask on all but the first deploy, but I think that's good — branch deploys should be rare anyway.

phocks · 2022-11-08T04:07:21Z

Implemented a warning if the FTP check fails when generating a new project due to no network or no credentials.

⣾⢷⡾⢷⡾⣷ aunty
⢿⡾⢷⡾⢷⡿ generate project

? What is your project called? (New Project)

 Error: getaddrinfo ENOTFOUND contentftp.abc.net.au
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:109:26) {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: 'XXXXXXXXXXXXXXX'
}

Warning: Unable to check if project name already exists, most likely due to a connection or credentials error. Please check manually before deploying.

? What is your project called? New Project
? What type of project is it? (Use arrow keys)
❯ Basic
  Preact
  React
  Svelte

phocks · 2022-11-11T04:28:16Z

Okay, I think this is pretty much ready to go. Simplified the process of getting credentials. Talked over all the functionality with @colingourlay

Tested and working.

jtfell

Code all looks good to me. I think it makes sense to prioritise being cautious about changing the ftp deploy process over reducing npm deps.

👍 👍

drzax · 2022-11-13T23:44:59Z

src/cli/deploy/index.js

+
+          const overwriteSelection = (
+            await cliSelect({
+              defaultValue: 0,


Not sure how cliSelect config works, but it seems like 'Yes' might be the default here? 'No' would be a safer choice.

Yep, that was a deliberate choice as most of the time you'll want to do the overwrite so it's just a single keypress confirmation, but absolutely a No default would be safer. Personally, I use aunty build && aunty deploy quite a bit to make latest working changes to interactives available to others while in the dev process and then proxy my local dev server using "Resource Override" in the browser. I can default it to No though and we'll check how it feels with the extra downarrow keypress.

phocks added 3 commits November 2, 2022 15:27

Some small housekeeping

ff10d3e

Check true or false works

a5713bf

Basically working

4a09349

phocks requested review from colingourlay, drzax and jtfell November 3, 2022 03:48

need aunty init support?

551cb98

drzax reviewed Nov 3, 2022

View reviewed changes

Moving things around a bit

88c12fe

phocks added 7 commits November 8, 2022 14:10

Issue warning for no network and no credentials

0cda631

support aunty init

11542ca

Deploy confirmation basically working

c1e48a4

Retain ssh capability

8d0bf5f

Clean up jsdoc

9fe053c

rename async wrapper to->wrap

1ce414a

Simplify getting root dir and ftp credentials

8c2b548

phocks requested a review from drzax November 11, 2022 04:22

jtfell approved these changes Nov 11, 2022

View reviewed changes

drzax approved these changes Nov 13, 2022

View reviewed changes

Change default overwrite value

760992a

colingourlay approved these changes Nov 14, 2022

View reviewed changes

phocks merged commit fd3015f into main Nov 14, 2022

phocks deleted the ftp-exist-check branch November 15, 2022 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ftp exist check #178

Ftp exist check #178

phocks commented Nov 3, 2022

phocks commented Nov 3, 2022

drzax left a comment

drzax Nov 3, 2022

phocks Nov 4, 2022

phocks Nov 11, 2022

drzax Nov 3, 2022

phocks Nov 11, 2022

drzax Nov 3, 2022

phocks Nov 8, 2022

phocks Nov 11, 2022

phocks commented Nov 4, 2022

drzax commented Nov 4, 2022

phocks commented Nov 8, 2022 •

edited

phocks commented Nov 11, 2022

jtfell left a comment

drzax Nov 13, 2022

phocks Nov 13, 2022

Ftp exist check #178

Ftp exist check #178

Conversation

phocks commented Nov 3, 2022

phocks commented Nov 3, 2022

drzax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phocks commented Nov 4, 2022

drzax commented Nov 4, 2022

phocks commented Nov 8, 2022 • edited

phocks commented Nov 11, 2022

jtfell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phocks commented Nov 8, 2022 •

edited