feat: Add port number feature to build command

[plato79]

Status

READY

Description

Added the port number to the build option for #1327

Type of Change

• ✨ New feature (non-breaking change which adds functionality)
• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ❌ Breaking change (fix or feature that would cause existing functionality to change)
• 🧹 Code refactor
• ✅ Build configuration change
• 📝 Documentation
• 🗑️ Chore

[tomarra]

Hi @plato79 👋thanks for taking on the issue and opening a PR! It looks like there are some failures in CI so please take a look at the output and get everything to green before we start the review process. Thanks!

[plato79]

The only "problem" was the initialization of "vars" variable which I didn't change. I fixed it as final. It should now pass ci analyze step. Though some of the tests fails and I don't think it's because of my additions.

[plato79]

Now, it's past anlalyze.. Though there is an error in prod_server_builder test. This one is a bit complex though, while the fix is very easy.

The code below is the reason for the error.

prod_server_builder.dart:

    final vars = <String, dynamic>{
      'dartVersion': dartVersion,
      'port': port,
    };

    logger.detail('[codegen] running pre-gen...');
    await prodServerBundleGenerator.hooks.preGen(
      vars: vars,
      workingDirectory: workingDirectory.path,
      onVarsChanged: (v) => vars..addAll(v ?? {}),
    );

There is a vars variable which has default dartVersion which is "stable" and default port which is "8080". There is possibility additional variables are provided through mason. We add those to the current variables defined in vars we have here. It modifies the value if the same key is used so everything is alright.

...until no variables are sent to here. Which makes v null (it says it shouldn't happen but it happens). Why vhas null value I am not sure. I thought that code would execute only when there is a value in v but this is not the situation.

Now the fix is easy as you can see. Just adding a null check fixes. But this time flutter says I shouldn't add a null check to a variable which shouldn't be null at all. While the code above fixes the problem and causes no problem, it probably will pop up in the ci analyze section as error just like the previous `"vars" should be defined as final" warning appears as error in analyze.

Do you have any suggestion about this?

[plato79]

Now, it's past anlalyze.. Though there is an error in prod_server_builder test. This one is a bit complex though, while the fix is very easy.

The code below is the reason for the error.

prod_server_builder.dart:

    final vars = <String, dynamic>{
      'dartVersion': dartVersion,
      'port': port,
    };

    logger.detail('[codegen] running pre-gen...');
    await prodServerBundleGenerator.hooks.preGen(
      vars: vars,
      workingDirectory: workingDirectory.path,
      onVarsChanged: (v) => vars..addAll(v ?? {}),
    );

There is a vars variable which has default dartVersion which is "stable" and default port which is "8080". There is possibility additional variables are provided through mason. We add those to the current variables defined in vars we have here. It modifies the value if the same key is used so everything is alright.

...until no variables are sent to here. Which makes v null (it says it shouldn't happen but it happens). Why vhas null value I am not sure. I thought that code would execute only when there is a value in v but this is not the situation.

Now the fix is easy as you can see. Just adding a null check fixes. But this time flutter says I shouldn't add a null check to a variable which shouldn't be null at all. While the code above fixes the problem and causes no problem, it probably will pop up in the ci analyze section as error just like the previous `"vars" should be defined as final" warning appears as error in analyze.

Do you have any suggestion about this?

Ok, that was not the cause. Still checking to see what the real problem here is.

[plato79]

This last commit should fix the error in the test.

[plato79]

The failing tests are actually runs successfully if you execute the commands outside of the test. For example build_test fails, but if you run dart run build/bin/server.dart in the temporary directory it created and call http://localhost:8080 it works. But the test itself cannot get a response from that page.

@tomarra Are you sure that these tests worked without problem before I made the change?

[plato79]

I think this is ready for merge. What do you think?

[alestiago]

Hi @plato79! I've added this to the review column for the next iteration (May 18 to May 31). Hopefully we get to review it by then 🙌

[tomarra]

Hi @plato79 👋

I got this PR updated with main and we are still seeing failures in the CI pipeline. In looking more closely at the output it actually looks like there are changes that are needed on the Docker side of things. While the Dart code looks like it solves the need the Dockerfile needs to be updated to reflect the port change. Pretty sure this is why the tests are failing to establish a connection with the container to run the tests.

[alestiago]

We do not manually update the bundles, can you revert this change? The bot will take care of updating the bundle after the PR is merged.

[alestiago]

Hi @plato79 this seems silent for a while, are you still able to complete the effort?

[plato79]

Sorry, because of my "paying" work, I couldn't concentrate on this for a while. I'll try to fix it tomorrow.