Term2 does not respect Pane Size

Term2 does not appear to respect pane sizes. I used atom 0.161.0 built from source. This is with the most recently published Term2 package (0.9.4), and I used an empty .atom besides this package.
As you can see, when I open a new term, tabs get pushed off the screen:

And the bottom of the pane gets pushed up:

The terminal lines themselves go way past the barrier of the pane, to the point where I can't see the cursor anymore:

This makes it pretty unusable, and I haven't quite figured out what's wrong.
I have no idea how to go about debugging this.

I'm having this issue too. Even more, when I resize, the terminal size appears right, but commands are pushed past the end of view, this too rendering quite unusable.

I am getting this as well. It adds a huge white pane to the bottom and makes all of my tabs at the top of the screen cut off.
It might also be good to have a little more documentation on how to change visual settings (css classes) so that things like this could be played with more. I had to guess and test CSS classes stuff just to up the font sizes.


More strict check for disable-errands task

Thanks for contributing to pcf-pipelines. To speed up the process of reviewing your pull request please provide us with:

A short explanation of the proposed change:
Currently disable-errands task can disable errands which you don't expect it to disable
An explanation of the use cases your change solves:
Try to disable errands in p-redis tile, e.g.

errands_to_disable: "register-broker,on-demand-broker-smoke-tests,upgrade-all-service-instances"

disable-errands task will also disable smoke-tests errand

Expected result after the change:

echo broker-registrar smoke-tests register-broker on-demand-broker-smoke-tests upgrade-all-service-instances broker-deregistrar delete-all-service-instances-and-deregister-broker | jq --arg to_disable 'register-broker
on-demand-broker-smoke-tests
upgrade-all-service-instances' --raw-input --raw-output 'split(" ")
    | reduce .[] as $errand ([];
       if $to_disable | split("\n") | index($errand) then
         . + [$errand]
       else
         .
       end)
    | join("\n")'
register-broker
on-demand-broker-smoke-tests
upgrade-all-service-instances

Note: I also tried to use regex for this but I faced issue with jq - https://github.com/stedolan/jq/issues/1629

Current result before the change:

echo broker-registrar smoke-tests register-broker on-demand-broker-smoke-tests upgrade-all-service-instances broker-deregistrar delete-all-service-instances-and-deregister-broker | jq --arg to_disable 'register-broker
on-demand-broker-smoke-tests
upgrade-all-service-instances' --raw-input --raw-output 'split(" ")
    | reduce .[] as $errand ([];
       if $to_disable | contains($errand) then
         . + [$errand]
       else
         .
       end)
    | join("\n")'
smoke-tests
register-broker
on-demand-broker-smoke-tests
upgrade-all-service-instances

Links to any other associated PRs or issues:

[x] I have viewed signed and have submitted the Contributor License Agreement

[x] I have made this pull request to the master branch

[x] I have run all the unit tests

Hi @antonsoroko thank you for creating this issue. Could you please clarify how you ran into this issue, which pipeline you used, did you make modifications to the pipeline, for example. We currently do not offer a pipeline to install service tiles, nor is there a feature in which you can disable errands for service tiles. The upgrade-tile pipeline (which does patch updates of PAS and service tiles, allow you to enable or disable errands - not to choose which ones specifically to disable). Thanks for helping us triage the issue. We also have the following pull request scheduled for engineering review: https://github.com/pivotal-cf/pcf-pipelines/pull/287.

@abbyachau
Hi. We use upgrade-tile pipeline with on-fly modifications (yaml-patch).
And because of our customer needs (currently they disable on-demand service brokers) we need to be able to specify which errands should be disabled.
I do not see any harm in this PR (this is a simple bug fix).
Thanks.


[dart2js] Clean up processing of @pragma('dart2js:...') annotations

We should rework the processing of @pragma annotations to address the following problems.
There are additional comments in the code-base referencing this issue.
MemberEntities only
dart2js's @pragma annotation processing is (mostly) keyed on MemberEntitys.

It is not a good fit for annotations that would be better placed on a library, class, or variable within a method.
It is not a good fit for annotations that we would like to be scoped, i.e. an annotation that applies to, say, methods, that can be placed on an enclosing structure (e.g. a class or library) to apply to all the methods within the enclosing structure.

J-model annotations
In lowering from the K-model to the J-model, we sometimes generate two (or more) J-entities for a K-entity. For example, a K-generative-constructor may be lowered to a J-generative-constructor-factory and a J-constructor-body.
Some annotations on the K-entity should apply to all the corresponding J-elements, others should apply to only one.
For example, noInline applied to a K-constructor reasonably applies to the J-generative-constructor-factory, but not the J-constructor-body that is called from the generative-constructor-factory. This happens by accident today, because the annotations are copied to only the primary J-entity. For noInline this is desirable - it would likely be bad for code size to reduce the number of calls to the constructor body and then prevent inlining at the reduced number of calls. By comparison, disable-inlining, which applies not to the method but to call sites inside the method, it is likely that the user intends to disable inlining inside the constructor's initializers and body.
To address the above example, we need to be able to say what the annotations mean in the J-model. I think a first try at this should probably formalize annotations as either an attribute of an entity, or an directive to be applied within an entity. The second kind are more naturally scoped.
Using pragmas in processing impacts
Pragmas that might naturally be scoped are used in places where the scope is not available. One of these is in processing impacts. Another is in some ssa optimizations (invoke_dynamic_specializer).
Rather than querying the annotations on the basis of an approximate representative member entity, we should find a way to flow the annotation scope to the place where the annotations are queried.
It is infeasible to use the Kernel AST node as a token for the scope of an operation where the annotations might have an effect - it deserializes too much of the .dill file.  We should investigate whether we could instead query the annotations at the beginning of the impact chain, in resolution where the Kernel AST is available, and propagate the answer rather than the index for the query (i.e. MemberEntity).  For example, today we have a TypeUse for a parametric covariant parameter type, perhaps this should become a TypeUse for a parametric covariant parameter type and whether the type has been 'trusted'.
pragmas and command-line settings
The default value of some pragma annotation queries is a bool property of the command-line options. It would be cleaner if these options imputed a set of scoped pragma annotations that apply to the 'whole program' scope. We could then explain that, e.g. -O3 is the same as a set of @pragma annotations applied to every file.

@rakudrama Are the @pragma annotations documented anywhere?

@mnordine, after your question I wrote some documentation at pkg/compiler/doc/pragmas.md.
I apologize for forgetting to let you know.

No worries! I seen it at the time.


InterruptedExceptionSwallowed.getThrownExceptions incorrectly handles catch blocks

Description of the problem / feature request:
InterruptedExceptionSwallowed.getThrownExceptions incorrectly handles catch blocks when there are multiple catch trees. As written, the function assumes each catch is capable of handling errors thrown by previous catch blocks, which is not the case.
Feature requests: what underlying problem are you trying to solve with this feature?
The bug checker should take into account how TryTrees are executed in java.
Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.
I will push a failing test shortly.
What version of Error Prone are you using?
df4cdefbf351c76098562c2d83eebf4cafaa8eea
Have you found anything relevant by searching the web?
No.

PR is up: https://github.com/google/error-prone/pull/1429

https://github.com/google/error-prone/commit/60de2dc1aa996d20263163d846d81ea2cbdc417f


#324 - changes Type to JComboBox with hardcoded values

Hope I didn't miss anything. I feel like those values should be available somewhere, but don't know where.
(Adding the default "float" to make the row visible. Very confusing otherwise and you tend to click on "Add" several times)
Fixes #324

Hmm, the VarTypes are openGL keywords. But then the parser probably has them as well.
However, I guess it's not too bad we're using a manual list.
One thing though: From the commit (and PR title) it's not clear that this is related to shader node variable type definitions


Fatal error: Uncaught GuzzleHttp\Exception\ClientException: Client error: POST https://oauth2.googleapis.com/token resulted in a 400 Bad Request response: { "error": "invalid_grant", "error_description": "Invalid JWT: Token must be a short-lived token

require DIR.'/vendor/autoload.php';
use Kreait\Firebase\Factory;
use Kreait\Firebase\ServiceAccount;
// This assumes that you have placed the Firebase credentials in the same directory
// as this PHP file.
$serviceAccount = ServiceAccount::fromJsonFile(DIR.'/critollfree-b3315-firebase-adminsdk-ssm2h-6da257d2a3.json');
// var_dump($serviceAccount);
$firebase = (new Factory)
->withServiceAccount($serviceAccount)
->withDatabaseUri('https://critollfree-b3315.firebaseio.com')
->create();
$database = $firebase->getDatabase();
$newPost = $database
->getReference('post')
->push([
'title' => 'Post title',
'category' => 'This should probably be longer.'
]);
$newPost->getKey();
$newPost->getUri();
$newPost->getChild('title')->set('Changed post title');
$newPost->getValue(); // Fetches the data from the realtime database
$newPost->remove();
I am working on localhost. i am getting following error.
Fatal error: Uncaught GuzzleHttp\Exception\ClientException: Client error: POST https://oauth2.googleapis.com/token resulted in a 400 Bad Request response: { "error": "invalid_grant", "error_description": "Invalid JWT: Token must be a short-lived token (60 minutes) and in (truncated...) in /Applications/XAMPP/xamppfiles/htdocs/tollfreeport/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php:113 Stack trace: #0 /Applications/XAMPP/xamppfiles/htdocs/tollfreeport/vendor/guzzlehttp/guzzle/src/Middleware.php(66): GuzzleHttp\Exception\RequestException::create(Object(GuzzleHttp\Psr7\Request), Object(GuzzleHttp\Psr7\Response)) #1 /Applications/XAMPP/xamppfiles/htdocs/tollfreeport/vendor/guzzlehttp/promises/src/Promise.php(203): GuzzleHttp\Middleware::GuzzleHttp{closure}(Object(GuzzleHttp\Psr7\Response)) #2 /Applications/XAMPP/xamppfiles/htdocs/tollfreeport/vendor/guzzlehttp/promises/src/Promise.php(156): GuzzleHttp\Promise\Promise::callHandler(1, Object(GuzzleHttp\Psr7\Response), Array) #3 /Applications/XAMPP/xam in /Applications/XAMPP/xamppfiles/htdocs/tollfreeport/vendor/kreait/firebase-php/src/Firebase/Exception/ApiException.php on line 41

The issue template is there for a reason :). Please update the issue with the requested information and please format the code samples so that they are readable.
Having parsed what you've given me so far, please check that the time in your environment is correct.

No issue with time in my environment. I am using the code as per instruction which are given in this link https://firebase-php.readthedocs.io/en/stable/
I have installed firebase admin sdk through composer
composer require kreait/firebase-php ^4.18
Can you please let me know what are the information should i send?
Thanks.

Sorry the issue is with time in my system which lags 5 mins. issue has been solved when i correct my computer clock.


Cannot get ref (useRef) for  wrapped inside a <Viewport.Tracker> using functional component

Cannot get ref for <FlatList /> wrapped inside a <Viewport.Tracker></Viewport.Tracker> using functional component
         const flatListRef = useRef();

        <Viewport.Tracker>
          <FlatList
            ref={flatListRef}
            data={chatMessages}
            renderItem={({ item: message }) => renderItem(message)}
            keyExtractor={(item) => `${item.id}-message`}
          />
        </Viewport.Tracker>

but when using a class component it works fine
        <Viewport.Tracker>
          <FlatList
            ref={(ref) => (this.flatListRef = ref)}
          />
        </Viewport.Tracker>

I have tried React.forwardRef but that doesnt seem to work either.
As soon as i remove <Viewport.Tracker> it works
Please can you assist?
kind Regards
Luke

Hi @bantingGamer,
To be honest, these components were never tested with hooks. They (still) work using a HoC approach and further more will try to hijack the ref of the inner component for their own use (i.e the HoC are not "clean").
We'v been thinking about implementing a hooks-based version of these but never had the time. If you like you could attempt to help there with a PR. @bevkoski  or @kern3lpan1c  might be also interested in helping out

Thank you for getting back to me, I really appreciate it.
I thought it wouldn't be the worst thing to refactor and use a Class Component.
Concerning your request for me to attempt to convert it to use hooks, is a little beyond my scope, Im a bit of a newbie when it comes to coding, I wouldn't even know where to begin :P.
Have a wonderful day
kind regards
Luke

Thank you too @bantingGamer, I will keep this open still.

Hey @brax10ward
to answer your question, i am not using createRef().
can you please provide sample code that isnt working, ideally a https://snack.expo.io/ would be awesome?
this should do the trick in a class component.
            <View>
              <FlatList
                ref={(ref) => (this.flatList = ref)}
                data={DATA}
                renderItem={renderItem}
                keyExtractor={(item) => item.id}
              />
              <Button
                title={'FlatList ref button'}
                onPress={() => {
                  console.log('flatList ref value: ', this.flatList);
                }}
              />
            </View>

Thanks for the response @bantingGamer. I will see if I can provide a sample on snack. Can you tell me how your are creating this.flatlist?

@brax10ward
I believe that this is the line where it is created.
 ref={(ref) => (this.flatList = ref)}
The this keyword is very confusing, I dont fully understand it myself that is why I prefer using functional components. You might want to brush up on how it works

@bantingGamer Can you share your whole component?

Any update on this? I am having the issue with react navigation 5, useScrollToTop(). It takes a ref of the flatlist, which is not working in a function component

@brax10ward sure here is the entire component snack example
you can just add

   <Viewport.Tracker>

...

   </Viewport.Tracker>

and it should work exactly the same

import React, { Component } from 'react';

import { View, FlatList, StyleSheet, Text, Button } from 'react-native';

const DATA = [

  {

    id: 'bd7acbea-c1b1-46c2-aed5-3ad53abbs28ba',

    title: 'item one',

  },

  {

    id: '3ac68afc-c605-48d3-a4f8-fbd91aass97f63',

    title: 'item two',

  },

  {

    id: '58694a0f-3da1-471f-bd96-145571esss29d72',

    title: 'item three',

  },

  {

    id: 'bd7acbea-c1b1-46c2-aed5-123546',

    title: 'item four',

  },

  {

    id: '3ac68afc-c605-48d3-a4f8-45678888',

    title: 'item five',

  },

  {

    id: '58694a0f-3da1-471f-bd96-1455767867867861e29d72',

    title: 'item six',

  },

  {

    id: 'bd7acbea-c1b1-46c2-aed5-fdds',

    title: 'item six',

  },

  {

    id: '3ac68afc-c605-48d3-a4f8-fbd91aa9kkukfuk7f63',

    title: 'item seven',

  },

  {

    id: '58694a0f-3da1-471f-bd96-23dfghh',

    title: 'item eight',

  },

  {

    id: 'bd7acbea-c1b1-46c2-aed5-6776899',

    title: 'item nine',

  },

  {

    id: '3ac68afc-c605-48d3-a4f8-111222',

    title: 'item ten',

  },

  {

    id: '58694a0f-3da1-471f-bd96-1234',

    title: 'item eleven',

  },

];

const Item = ({ title }) => (

  <View style={styles.item}>

    <Text style={styles.title}>{title}</Text>

  </View>

);

const renderItem = ({ item }) => <Item title={item.title} />;

class App extends Component {

  render() {

    return (

      <View

        collapsable={false}

        style={{

          flex: 1,

          alignItems: 'center',

          width: '100%',

          // backgroundColor: 'red'

        }}>

        <FlatList

          style={{

            height: 100,

          }}

          ref={(ref) => (this.flatList = ref)}

          data={DATA}

          renderItem={renderItem}

          keyExtractor={(item) => item.id}

        />

        <Button

          title={'press to scroll'}

          onPress={() => {

            this.flatList.scrollToIndex({ index: 0 }); // scroll a bit before pressing the button

            // or

            // this.flatList.scrollToOffset({ offset: 300 });

          }}

        />

      </View>

    );

  }

}

const styles = StyleSheet.create({

  item: {

    backgroundColor: '#f9c2ff',

    padding: 20,

    marginVertical: 8,

    marginHorizontal: 16,

  },

  title: {

    fontSize: 32,

  },

});

export default App;

I see that you are using it fine as a class component, but how can we make it work with a function component. When assigning a ref to the flatlist, it gets ignored inside the viewport wrapper.

Hi @bantingGamer,
To be honest, these components were never tested with hooks. They (still) work using a HoC approach and further more will try to hijack the ref of the inner component for their own use (i.e the HoC are not "clean").
We'v been thinking about implementing a hooks-based version of these but never had the time. If you like you could attempt to help there with a PR. @bevkoski or @kern3lpan1c might be also interested in helping out

@servonlewis this is currently an issue.

Hey guys,
for all you react hook users, I was able to use the native controls from within Flatlist to achieve a viewport checker.
if interested, below is the code i used to make it work.
// flatlist, requires viewabilityConfig and onViewableItemsChanged
<FlatList
      scrollEventThrottle={16}
      {...{
        onViewableItemsChanged,
        keyExtractor,
        data,
        onScroll,
        ref,
        renderItem,
        refreshing,
        onRefresh,
        viewabilityConfig,
      }}
    />

const viewabilityConfig = {
  waitForInteraction: true,
  itemVisiblePercentThreshold: 60, // I set this to 60 so that only one video plays, anything lower than 50 and both will play if 2 are in view
};

//use some state to capture the id of the viewport you are capturing
 const [videoPlaying, setVideoPlaying] = useState<number | null>(null);

//useCallback is import because this function cannot rerender or it'll error, as per docs.
//for me, i only wanted to capture the viewport of an item that has the video prop.
  const onViewableItemsChanged = useCallback(
    (obj: { viewableItems: ViewToken[]; changed: ViewToken[] }) => {
      const isPlaying = obj.viewableItems?.find((x) => {
        const item: Props = x?.item;
        return !!item?.video;
      });
      isPlaying
        ? setVideoPlaying((isPlaying?.item as Props)?.id)
        : setVideoPlaying(null);
    },
    []
  );

//pass that state to your component, then elsewhere, I created my video player component like this, where it only plays if the video prop, and the id matches.

interface Props {
  id: number;
  videoPlaying?: number | null;
}

export const VideoForCard = ({ id, videoPlaying }: Props) => {
  const [shouldPlay, setShouldPlay] = useState(false);
  const isFocused = useIsFocused();

  useEffect(() => {
    id === videoPlaying ? setShouldPlay(true) : setShouldPlay(false);
  }, [id, videoPlaying]);

  useEffect(() => {
    if (!isFocused) setShouldPlay(false);
    if (isFocused && id === videoPlaying) setShouldPlay(true);
  }, [isFocused]);

  return (
    <StyledVideo
      source={{
        uri: "http://d23dyxeqlo5psv.cloudfront.net/big_buck_bunny.mp4",
      }}
      rate={1.0}
      volume={1.0}
      resizeMode="cover"
      isMuted={true}
      useNativeControls
      {...{ preTriggerRatio, shouldPlay }}
    />
  );
};

I'm having the same issue with functional components.

I was able to get this working with hooks, by using the following syntax:
<ScrollView ref={ref => (scrollView.current = ref)}

instead of
<ScrollView ref={scrollView}>

🙌

This worked for me
  const flatList = useRef();
...
      <FlatList
        ref={flatList}
...
 onPress={() => flatList.current.scrollToIndex({ index: 1 })}
...


[Request]: Only Embedded Documentation (Disable Dedicated Panel)

What feature would you like to add?
I'm using this plugin to render documentation within my admin panel, but do not necessarily want the docs panel enabled. It would be great if there was a way to simply disable it altogether without losing the rest of the plugin's functionality.
Notes
No response

You can however forbid access to the panel using the canAccessPanel method of your User model. Just return false for the Knowledge Base panel.


Confusing Table Names

Issue
The tutorial uses 2 tables, named Bakery and Baker to demonstrate various concepts - these names are extremely similar to each other, and it's making reading some of the code snippets demonstrating things more difficult.
Renaming one of them would solve the problem - perhaps changing baker to cook, chef or even patissier?
Example
An area which is particularly awkward for reading is the Migrations file definitions on this page: https://www.sea-ql.org/sea-orm-tutorial/ch01-02-migration-cli.html
This section, particularly around the foreign key definition is very difficult to read:
    async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> {
        manager
            .create_table(
                Table::create()
                    .table(Baker::Table)
                    .col(
                        ColumnDef::new(Baker::Id)
                            .integer()
                            .not_null()
                            .auto_increment()
                            .primary_key(),
                    )
                    .col(ColumnDef::new(Baker::Name).string().not_null())
                    .col(ColumnDef::new(Baker::ContactDetails).json())
                    .col(ColumnDef::new(Baker::BakeryId).integer().not_null())
                    .foreign_key(
                        ForeignKey::create()
                            .name("fk-baker-bakery_id")
                            .from(Baker::Table, Baker::BakeryId)
                            .to(Bakery::Table, Bakery::Id),
                    )
                    .to_owned(),
            )
            .await
    }

Hey @Isona, welcome!! You raise a good point and it's definitely worth the effort to rename Baker to something else, do you think Chef be appropriate?
Thoughts? @shpun817 @tyt2y3

Sure! That's a good point indeed.

Hey @shpun817, would you like to make the changes when you have time?

Okay, it shouldn't be too hard I guess.


[WIP] Log when GCR requests exceed quota

What type of PR is this?
/kind feature

What this PR does / why we need it:
This change logs a warning when the auditor is observed to make more HTTP requests than the GCR quota: 50,000 requests per 10min. In this scenario, the auditor is expected to be throttled by GCR and experience longer response times from each request. This behavior should be avoided at all costs, since throttling will also slow the verification process of each GCR change. In the worst case, this slowdown may leave multiple Pub/Sub messages unacknowledged causing a backup of verifications and cause alerts to fire. This change looks to gain visibility into when, if at all, this behavior occurs.
Which issue(s) this PR fixes:
Partially satisfies #358

Special notes for your reviewer:
This warning will only occur if the request counting is enabled in the first place. Therefore, the auditor must be run with the --verbose flag to gain such visibility.
Does this PR introduce a user-facing change?
NONE

Log a warning when the number of GCR requests exceeds quota.

cc: @listx @amwat @justaugustus @kubernetes-sigs/release-engineering

Add "error code" to make filtering logs easier

Closing in favor of #384


RBAC support added to CLI Migrate

Description
Changes proposed in this pull request:

RBAC section is now shipped to Cloud on migrate command

Testing

Run cloud app locally
Deploy botkube in local k8s cluster
Go to cli folder cd cmd/cli
Build CLI go build -o bctl main.go
Login to local cloud ./bctl login --cloud-dashboard-url http://localhost:3000
Migrate deployment ./bctl migrate --cloud-api-url http://localhost:8080/graphql

Related issue(s)
Relates https://github.com/kubeshop/botkube-cloud/issues/99

@huseyinbabal Please also update this document https://docs.botkube.io/cli/migrating-installation-to-botkube-cloud
also as agreed on Slack, please describe that the extraObjects are not migrated. So the helm upgrade that we perform simply removes all of them, so if user had cluster roles or bindings there, then they need to migrate them on their own to make sure that migrated plugin RBAC config will work properly.

@huseyinbabal Please also update this document https://docs.botkube.io/cli/migrating-installation-to-botkube-cloud
also as agreed on Slack, please describe that the extraObjects are not migrated. So the helm upgrade that we perform simply removes all of them, so if user had cluster roles or bindings there, then they need to migrate them on their own to make sure that migrated plugin RBAC config will work properly.

https://github.com/kubeshop/botkube-docs/pull/254


gopass -c  doesn't auto clear on KDE Plasma5

I think pass does qdbus org.kde.klipper /klipper org.kde.klipper.klipper.clearClipboardHistory &>/dev/null, but I don't see anything like this in gopass..
gopass 1.5.1 (10c2866255a2bfd40cc9f328ed1101d934e11b77) go1.9 linux amd64

Thank you for the hint, we will check that.

I'm so amazed at how quickly you responded to this. Just tried it out and it works great.
FYI: Plasma5 default clipboard settings have a history of 10 clipboard records and the default pass/gopass behaviour is to clear the entire record. This is not necessarily bad, since trying to do anyting else can quickly get messy. I don't even know if the plasma5 dbus interface allow for removal of a certain clipboard record and if there is, removing the latest clipboard entry will not necessarily contain the password added by gopass.
You might want to inform your KDE users in your documentation.
Thank you so much for your work!

Thanks for trying it out already!
I've added a small note to our FAQ about clearing the whole klipper history.


Resolve FPs reported under issue 6

Closes https://github.com/coreruleset/nextcloud-rule-exclusions-plugin/issues/6
I also fixed a few additional FP I noticed while working on this issue

Considering that @AlroneRhyn has not responded in weeks and is unlikely to respond to this PR now I'll merge this.

fair enough, I'll open another PR if he does report any issues or if I discover issues.


[ECS] [Container OOM]: Containers OOM with Amazon Linux 2023 ECS AMI

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

Tell us about your request
ECS Containers are getting killed due to Out of Memory with new Amazon Linux 2023 ECS AMI.
Which service(s) is this request for?
ECS - with EC2 (Autoscaling and Capacity Provider setup)
Tell us about the problem you're trying to solve. What are you trying to do, and why is it hard?
We are deploying our workloads on AWS ECS (EC2 based). We recently migrated our underlying cluster instances AMI to Amazon Linux 2023 (previously using Amazon Linux 2). After the migration, we are facing a lot of "OOM Container Killed" for our services without any change on the service side.
Are you currently working around this issue?

By increasing hard memory limit for now (we have more then 600 services , it's hard to do that for each of that)
Rolling back to Amazon Linux 2 AMI

Hello, I have transferred this issue to the ECS/EC2 AMI repo from containers-roadmap, since this sounds more like it could be a bug or change in behavior in the AMI, rather than a feature request.
@rixwan-sharif could you let us know which AL2023 AMI version you used? Was it the latest available? Could you also provide the task and container limits that you have in your task definition(s)?
Two differences that come to mind that may be relevant are the latest AL2023 AMI is using Docker 25.0 and cgroups v2, whereas the latest AL2 AMI is currently on Docker 20.10 and cgroups v1.

If you were not using the latest AL2023 AMI, one thing to note is that the Amazon Linux team released a systemd fix in late-September 2023 for a bug in the cgroup OOM-kill behavior. (Search "systemd" in release notes here: https://docs.aws.amazon.com/linux/al2023/release-notes/relnotes-2023.2.20230920.html)

If anyone has data to provide to the engineering team that can't be shared here, please feel free to email it to ecs-agent-external (at) amazon (dot) com, thank you :)

Hi, This is the AMI version we are using
AMI Version : al2023-ami-ecs-hvm-2023.0.20240409-kernel-6.1-x86_64
Task/Container details
Base Docker Image: adoptopenjdk/openjdk14:x86_64-debian-jdk-14.0.2_12
Application Framework: Java/Sprint Boot
Resources:
CPU : 0.125
Memory(hard): 1GB
Docker stats on Amazon Linux 2 AMI.

Docker stats on Amazon Linux 2023 AMI. (Increased Memory hard limit to 3GB as container was OOMing with 1GB of memory)

And yes we already opened a support case too (Case ID<PHONE_NUMBER>00518). this is what we got from support.
[+] Upon further troubleshooting, we found that there seems to be issue with AL2023 AMI which our internal team is already working on and below are wordings shared by them:
We are following up on your inquiry regarding increased container OOM (out-of-memory) kills when using ECS-Optimized AL2023 AMI, comparing to AL2 AMI. The team is investigating to identify what has caused the OOM kill behavior change. We suspect a combination of Cgroupv2 and container runtime updates. As a workaround, we recommend that customer adjust container and task memory hard limit to have a larger buffer, based on their container memory usage patterns, using ECS Container Insights metrics.

Hi @sparrc, after switching to AML 2023 from AML 2 we faced w/ a similar issue as well, we haven't got OOM yet but the memory consumption has nearly doubled, and memory consumption regularly increases as the application runs which looks like a memory leak.
AMI Version: al2023-ami-ecs-hvm-2023.0.20240515-kernel-6.1-x86_64
Base Docker Image: python:3.8-slim
Application Framework: Python / Django / uwsgi

Also experiencing the same behavior with the latest AMI:
AMI al2023-ami-ecs-hvm-2023.0.20240712-kernel-6.1-x86_64
Going from AL2 to AL2023 results in significant memory consumption increase (seems generalized regardless of language/framework)
If the ECS internal team is aware of this is there somewhere where we can track it? It seems like this has been an issue for months and would like to stay current on any progress or notifications.

We have memory limits set at the TaskDefinition level and the JVM is allocating heap based on the total physical RAM on the host system now, which is blowing up our RAM usage.  We were able to set memory limits on the individual ContainerDefinitions within the TaskDefinition and that seems to have fixed it for us.

We ran into the same issues, our Java services were either freezing or hitting OOM errors after the upgrade to AL2023. It turned out we were running a JDK version which did not have support for Docker cgroups v2, meaning the JDKs inside the containers were not aware of the memory limits imposed by ECS tasks. Upgrading the JDK to a recent version with cgroups v2 support fix the issue

@dduvnjak Interesting. I've had issues too, however this is running Java 21.0.5 so it can't be the same as your root cause.
Since I dont have good metrics before/after the change I can't really assume the memory usage is greater, so I'd just come to the naive conclusion that the memory killer is just far more aggressive with ECS AMI AL2023 and cgroups v2 and perhaps instantaneous blips over the max (which come down shortly after) are being punished more severely and instantly than before. I can't find good articles, but I was under the understanding that this was one of the design goals in cgroups v2 to improve isolation (requiring punishing containers over maxes more strictly).
I suppose it's possible that even on a JDK with cgroups v2 support the ergonomics and values it uses differ slightly causing it to consume more though...
Strangely enoguh at https://docs.aws.amazon.com/linux/al2023/ug/ecs.html it notes

All processes in the cgroup were always killed instead of the OOM-Killer choosing one process at a time, which is the intended behavior.

Actually, I thought that killing everything in the cgroup/container was one of the intentions/goals for containers with cgroups v2.... (but can confirm that on AL2023 ECS AMIs it is just picking a process inside my container to kill, which leads to unpredictable behaviour)

I have an issue due to run datadog-agent daemon sets with 512MB hard limits memory.
Which is before migrating from AL2 to AL2023 this problem never happened, So we migrate back to al2

I wonder how the experience differs between Fargate ECS users and old-school EC2 ones. Mine is non-fargate for what it's worth.

For my experience the fargate it self quite convenient for whom doesn't wanna concern the infrastructure but it come with costly if you compare with ec2 it self
But it has fargate spot that very cheap, if your workload has falut torelant ability it is a good candidate
By the way there is limitations when using fargate like you cannot using adjust or using host capability.


Improve Word Cloud design

Modify Font size and packing to make the word cloud more interesting

Need to  also make the Word Cloud responsive in terms of filling in the container. Dynamic Padding and Font-size, or Viewbox approach should work.


Error: Your IP not found

I get this error when I attempt to close a thread. What do I need to do?

Okay, so I have read some issues that have been closed regarding this case, and I cannot find the option to change the IP... Where may I find this?


Where can I get the pretrained model of the mushroom?

Where can I get the pretrained model of the mushroom?Thank you!

@MysteryNull Are you referring to the models trained for object detection (det)? You can check the metafile.yaml in each algorithm folder under the "configs" directory.

@MysteryNull Are you referring to the models trained for object detection (det)? You can check the metafile.yaml in each algorithm folder under the "configs" directory.

the COCO dataset trained in your model contains objects365 v1 dataset？


Chore: Update spacing/indents/conventions

Need to go through all notes and update items based on conventions based on different style guides.

[x] Python: PEP-0008
[ ] C++: Cpp Core Guidelines
[ ] Java: Twitter Style Guide
[ ] JavaScript: ?
[ ] Rust: ?
[ ] Swift: ?
[ ] etc. ?

Help requested:
If you know of a style guide that is a convention for other languages, please post them here so that I can collate and complete this chore eventually.

Hi Mr.Seidel. If you want, I can update all the indents to conform to PEP-8  in my existing pull request

Hi Mr.Seidel. If you want, I can update all the indents to conform to PEP-8  in my existing pull request

Hi Mr.Seidel. If you want, I can update all the indents to conform to PEP-8 in my existing pull request

No, I would want this as a separate pull request since it is a different topic and so that I can easily find it in the future.

Hi Mr.Seidel. If you want, I can update all the indents to conform to PEP-8 in my existing pull request

No, I would want this as a separate pull request since it is a different topic and so that I can easily find it in the future.

Hi Mr.Seidel. If you want, I can update all the indents to conform to PEP-8 in my existing pull request

No, I would want this as a separate pull request since it is a different topic and so that I can easily find it in the future.

I've updated all the python examples to follow PEP-8 Indentation guideline (#31)

Hi Mr.Seidel. If you want, I can update all the indents to conform to PEP-8 in my existing pull request

No, I would want this as a separate pull request since it is a different topic and so that I can easily find it in the future.

I've updated all the python examples to follow PEP-8 Indentation guideline (#31)

Some style guides for the languages listed (as well as for Go):

JavaScript: Google Style Guide
Rust: Style Guide
Swift: Google Style Guide (hmm)
Go: Google Style Guide


render multiple lines in git commit message preview under Source Control

If I put newlines in my git commit message, it is not rendered as multiple lines but concatenated into single line..
I need to put
 in my commit message to render it as multiple lines... But of course... It won't look good in git log command line if there are extra
..
Shouldn't VSCode do something like commitMessage.replace(/(?:\r?\n|\r)/g, '
') before rendering the commit message?
This should handle \n, \r\n, and \r ...
If my suggestion is okay, can someone points out the location to put this change so I can get one commit into vscode ;-)

Sorry! It looks like the "COMMITS" view I'm referring to belongs to GitLens and the "TIMELINE" view from VSCode shows multiple lines properly!
I'll check with GitLens project.

/causedByExtension

This issue is caused by an extension, please file it with the repository (or contact) the extension has linked in its overview in VS Code or the marketplace for VS Code. See also our issue reporting guidelines.
Happy Coding!

Just to provide a cross-reference. This is probably related to https://github.com/gitkraken/vscode-gitlens/issues/2045


Test for 00ea30f which fixes #875

Test for 00ea30f which fixes #875

Hi! Thanks for the PR and sorry for the delay. Would you mind rebasing? If it passes after that, I'll merge.

I don't have PC access, can you please enable this option for the repository:

... so it can be done from the GitHub UI with a Click/Tap?

Done!

Sure, agree, it's just way easier with this button:

Being able to rebase also allows for fixing on GitHub UI on mobile, so this is ready to merge after a re-review.
Rebase was clean as there were no conflicts, but it failed as you expected.


Patch 1

source code of paper "What Do They Capture? - A Structural Analysis of Pre-Trained Language Modes of Source Code"


net: can't unwrap DNSError

What version of Go are you using (go version)?

$ go version
go version go1.21.1 darwin/arm64

Does this issue reproduce with the latest release?
YES.
What operating system and processor architecture are you using (go env)?
go env Output
$ go env
GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/budougumi0617/Library/Caches/go-build'
GOENV='/Users/budougumi0617/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/budougumi0617/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/budougumi0617/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.21.1/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.21.1/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/ll/hpp009q97nvdhy4j49dmw96h0000gn/T/go-build623399656=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?
The release notes for Go 1.19 mention the following specification about the "operation was canceled" error in the net package:

When a net package function returns an "operation was canceled" error, the error will now satisfy errors.Is(err, context.Canceled).

However, when I execute errors.Is(err, context.Canceled) against the "operation was canceled" error that returns from methods like *Dialer.DialContext, it does not evaluate to true. Is this in accordance with the specification?
https://go.dev/play/p/Os-LQrrlUx6
	_, err := (&net.Dialer{}).DialContext(ctx, "tcp", "localhost:12345")

	t.Logf("error was %q\n", err) // error was "dial tcp: lookup localhost: operation was canceled"

	if !errors.Is(err, context.Canceled) {
		t.Errorf("%T, was not context.Canceled", err) // *net.OpError, was not context.Canceled
	}

Looking at the execution result, it appears that the *net.DNSError within the *net.OpError has not implemented the Unwrap method. If the Err field in the DNSError struct retained its value as an error type, I believe the Is method of errCanceled (of type canceledError) from the net package would be called, and errors.Is(err, context.Canceled) would return true.
https://pkg.go.dev/net#DNSError
type DNSError struct {
	Err         string // description of the error
	Name        string // name looked for
	Server      string // server used
	IsTimeout   bool   // if true, timed out; not all timeouts set this
	IsTemporary bool   // if true, error is temporary; not all errors set this
	IsNotFound  bool   // if true, host could not be found
}

https://github.com/golang/go/blob/go1.21.1/src/net/net.go#L411-L422
	// For both read and write operations.
	errCanceled         = canceledError{}
	ErrWriteToConnected = errors.New("use of WriteTo with pre-connected connection")
)

// canceledError lets us return the same error string we have always
// returned, while still being Is context.Canceled.
type canceledError struct{}

func (canceledError) Error() string { return "operation was canceled" }

func (canceledError) Is(err error) bool { return err == context.Canceled }

ref: https://github.com/golang/go/issues/51428
ref: https://github.com/golang/go/issues/36208

What did you expect to see?
When a net package function returns an "operation was canceled" error,  errors.Is(err, context.Canceled) returns true.
What did you see instead?
When a net package function returns an "operation was canceled" error,  errors.Is(err, context.Canceled) returns false.

The problem, as your test shows, is that net.DNSError does not support unwrapping to an underlying error.  I don't know how easy that will be to fix while retaining backward compatibility.

I've made a proposal to fix this issue #63116.

Duplicate of #63116

I appreciate your quick triage and proposal creation!


Weak Announcement Support

Special notes:

Only #when:send:to: is supported for Weak Announcement registration
Weak BlockClosure instances are not supported and will result in a raised WeakBlockUnsupported Error

Reviewed and "merged" to development. Will merge to main along with RSR/Sparkle repos following integration testing.


Avoid duplicating warning in ng-repeat

Add " '~' + $index" in the following line:
<li ng-repeat = "row in tree_rows | filter...track by row.branch.uid || '~' + $index">
When row.branch.uid is empty, ng-repeat error will occur: "Duplicates in a repeater are not allowed".

Fix the variables of the example in README.
<abn-tree tree-data         = "my_treedata" ...>
$scope.my_treedata = [{ // Replace 'my_data' with 'my_treedata'
  label: 'Languages',
  children: ['Jade','Less','Coffeescript']
}]

Add repository and fix license in package.son (licence -> license).

uid should never be empty so I'm not sure if this change is necessary; however, the other cleanup items are still valid.


Catch ODE from bedrock framework code

Fixes https://github.com/dotnet/tye/issues/776
As we plan to remove bedrock.framework long term in favor of YARP, just going with the bandaid solution of catching the ODE from bedrock framework.

YARP doesn't replace bedrock 😄

Can you reproduce this?

Can you reproduce this?

Infrequently, but I can occasionally.

YARP doesn't replace bedrock 😄

Why wouldn't it? Don't we want the same behavior locally as we do on kubernetes? I don't think we need L4 capabilities...

I investigated this for around a day in the past and wasn't able to get a repro with a locally built version of the package (release mode, with logs, etc). I think that's an appropriate time box for it.

Why wouldn't it? Don't we want the same behavior locally as we do on kubernetes? I don't think we need L4 capabilities...

We use L4 for replicas, which is what k8s does.

We use L4 for replicas, which is what k8s does.

Ah, you are talking about the case where there isn't an ingress. This code is still used I guess then.
Do you think long term we should keep this dependency on Bedrock.Framework?

Ah, you are talking about the case where there isn't an ingress. This code is still used I guess then.

No I mean when there are multiple replicas of a service, the address you get is proxy address. That's not an HTTP proxy and has nothing to do with the ingress.

Yeah, I was agreeing with you.

offline approval


Update report detail type

Update of report detail type to support PGGD ingest.
Contents may extend beyond what is required for 'balance form geoproperties' but has been included for completeness.
Suggest Address fields be concatenated into a single value for the purposes of these forms.

Created from wrong branch. Will redo


Short variable declaration not working with array dereferencing

What version of Go are you using (go version)?

$ go version
go version go1.12.1 darwin/amd64

Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (go env)?
N/A
What did you do?
package main

import ("errors";"fmt")

func main() {
	var s [1]int
	s[0], err1 := 1, errors.New("some error")  // error: non-name s[0] on left side of :=
	fmt.Printf("%v\n", err1)

	var err2 error
	s[0], err2 = 1, errors.New("some error") // all good
	fmt.Printf("%v %v\n", s[0], err2)

	var i int
	i, err3 := 1, errors.New("some other error") // all good
	fmt.Printf("%v %v", i, err3)
}

You can try it out here: https://play.golang.org/p/OIy7TJWnSkF
What did you see?

if I change the operation to multiple assignment (pre-initialise err2), everything works as expected.
if instead of s[i] (dereferencing an array) I use a single value (var i int), the mixed assignment/short declaration works as advertised.

What did you expect to see?

at best I would expect Go to compile the program (not an unreasonable expectation IMO; given that multiple initialisation works, I don't see why short var declaration wouldn't).
at the very least I would expect a better compiler error.

This is a well known problem in Go:

https://github.com/golang/go/issues/30318
https://github.com/golang/go/issues/377

Like @go101 said, see the previous issues and proposals.


⚠ Bofrost.be has degraded performance

In e37cc5d, Bofrost.be (https://www.bofrost.be) experienced degraded performance:

HTTP code: 200
Response time: 1491 ms

Resolved: Bofrost.be performance has improved in d5bc8e4 after 6 minutes.


lib.systems: Avoid allocating intermediate attrset when parsing skeleton list

Description of changes

Things done

Built on platform(s)

[ ] x86_64-linux
[ ] aarch64-linux
[ ] x86_64-darwin
[ ] aarch64-darwin

For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)

[ ] sandbox = relaxed
[ ] sandbox = true

[ ] Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test
made sure NixOS tests are linked to the relevant packages

[ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
[ ] Tested basic functionality of all binary files (usually in ./result/bin/)
23.11 Release Notes (or backporting 23.05 Release notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[ ] Fits CONTRIBUTING.md.

Priorities
Add a :+1: reaction to pull requests you find important.

Do we do this frequently enough to care about that allocation cost? I wouldn't think so.

Do we do this frequently enough to care about that allocation cost? I wouldn't think so.

Not really, no. But it looks ugly for no reason.

I prefer "switch case" to "chained if" :)

I think { "1" = ...; }.${toString (length l)} constructs like is an anti-pattern & an eye sore that possibly encourages others to build in the same style.
It doesn't matter (much) for performance in this instance, but it does in general.

Yeah it's not as nice for numbers as for string version, but I still think it is a fine style because it is closer to pattern matching, and pattern matching is good.
Maybe someone else will chime and and agree with you I'm being nuts :)

Since this is just cleanup, could it please wait for

https://github.com/NixOS/nixpkgs/pull/235230

to merge first?  That PR contains the test suite for the code that's being modified here -- right now we have no test coverage for it at all.

Yeah, no worries.

Can't be bothered arguing about this one.


remove calling array_keys in foreach loops

:+1:
I would be nice if PHP 7 would support
foreach($array as $key =>;) {
    //
}

but then again ;)


Godot 3.0 crashes automatically on the OSX platform crash

gdoot 3.0
Be careful:(Drag the mouse quickly, change the window size)
OSX platform, quickly and repeatedly adjust the window, Godot window automatically crashes, automatic exit.

The game window or the editor window ?

@groud  editor window

I just tried to reproduce it here but it's not crashing for me, is there any info on the console? Crashes for you when adjusting the editor window with any project?

Not sure if I tried this right but on a build I pulled from master earlier today I can't get it to crash.
2012 Macbook Pro Retina with an Intel 4000

@BastiaanOlij A quick adjustment window can crash.

Has been repaired


fix(HLS): Skip EXT-X-PRELOAD-HINT without full byterange info

Related to https://github.com/shaka-project/shaka-player/issues/5089

We need investigate another solutions, for example https://www.akamai.com/blog/performance/-using-ll-hls-with-byte-range-addressing-to-achieve-interoperabi


update compatible redmine version to 4.2

Plantuml installation fails in Redmine 4.2.1 environment

bundle exec rake redmine:plugins:migrate RAILS_ENV=production
rake aborted!
Redmine::PluginRequirementError: plantuml plugin requires a Redmine version between 2.6 and 4.1 but  current is 4.2.1.stable

modify plantuml/init.rb
requires_redmine version: '2.6'..'4.1'
to
requires_redmine version: '2.6'..'4.2'


fix: unbreak main workflows

What this PR does / why we need it:
Fixes main workflows by:

fixing a compilation problem that was caused by concurrent merges of https://github.com/Kong/kubernetes-ingress-controller/pull/6619 and https://github.com/Kong/kubernetes-ingress-controller/pull/6585
checking out PR references in reusable workflows called from checks.yaml that uses pull_request_target

We'll have to forcefully merge it as otherwise we won't be able to fix main workflows.


Is there a plan to support fixed32?

Is there a plan to support fixed32?

Supporting Fixed32 is an interesting idea and could be beneficial in scenarios where memory efficiency is crucial, such as mobile applications or games. Currently, FixedMathSharp leverages Fixed64 (the underlying value is a long, which is a 64-bit value, hence the name) to ensure high precision and a larger range for computations. I built this specifically for a deterministic physics engine.
Adding Fixed32 would require careful consideration of range and precision trade-offs, as a Fixed32 would inherently have a smaller range and lower precision than Fixed64. I’d need to rework some of the logic and rounding methods to suit the 32-bit format while maintaining accuracy.
I'm open to exploring Fixed32 support if there’s enough interest. If you or the community would benefit from this, please feel free to reach out or contribute ideas. I’d love to hear more about specific use cases where Fixed32 would be a valuable addition!


symfony/templating is deprecated

Bug Report

Q
A

BC Break
yes

Version
all

Summary
Symfony has deprecated the Templating component in 6.x and it's been removed in 7.0. In order to support Symfony 7 we'll need to update the usages of the Templating component to Twig.

I have tried and removed "implements Helper" from UploadHelper altogether (as the Helper only has a getter and setter for Charset which is not used in this bundle, and everything seems to work fine.

That's great @knallcharge! Would you be able to submit a PR with the change?

I have already included all the other sf7-fixes from this fork (https://github.com/evertharmeling/OneupUploaderBundle/tree/sf-7) into mine, so I'll have to setup a new one for it. Basically it's just removing the interface-implementation, but I can look into probably sometime on Monday.

I just created a PR: https://github.com/1up-lab/OneupUploaderBundle/pull/438
Note that all sf7-fixes from evertharmeling are also included so credit for that goes to him. I can't create another fork with just the templating-change without deleting my old one which is currently in use, therefore the combined PR.
Not using all featues of the bundle this works for me but a close look is recommended. Also this may break custom filesystem-implementation that may have used the get/setCharset-methods.

What if I merge the changes from @evertharmeling and you can create a follow up PR? I like to keep thing simple and keep the credits.

Sure, good idea.

@knallcharge I've merged the PR from @evertharmeling 👍


Highlight locally modified files in Reviewer

Make UI highlight these files:

Locally modified, uncommitted files
Committed but not yet pushed - a boolean field should be added to notify the UI. We will later set that field correctly in the local server.

 Highlight locally modified files in Reviewer


Fix for chmod 777

Task name: DockerInstallerV0, KubernetesV0
Description: changing the chmod of DockerInstallerV0 and KubernetesV0 from 777 to 755
Documentation changes required:
Added unit tests: (Y/N)
Attached related issue: (Y/N) https://mseng.visualstudio.com/AzureDevOps/_workitems/edit/1983871
Checklist:

[x] Task version was bumped - please check instruction how to do it
[x] Checked that applied changes work as expected

Same chmod was updated previously with #17547.
Adding here just for reference, ContainerBuildV0, KubernetesV0, KubernetesV1 might need same change.

/azp run


Fixes memleak

This fixes the memory leak found in #178 .
Fortran was allocating memory for an options array of variable length, but never deallocating it.  I thought memory for allocatable arrays was automatically deallocated, but I guess valgrind doesn't think so.  So I just allocated memory at compile time, with a check to make sure mwaves never exceeds 20 (size of static array).

Cool! That filament regression is a but annoying in the long run. Nothing to do with this one. ;)


Copy / Paste image

Hello,
Does it possible to implement copy / paste image ?
Regards

This shouldnt be too difficult to implement, we are using a custom built image plugin for TipTap so It should just be a case of adding a file drop / paste handler.


Fix etcd install failure when re-run

The creates parameter on unarchive module causes it to be skipped when
re-run after a successful install.  However, the next copy step tries to run
again even though the downloads are not present.  This change will not
run the step to "move binaries into path" if the previous step is
skipped.

@craigtracey for review


Support both wallet_switchEthereumChainObject and wallet_addEthereumChain JSON-RPC methods in WalletConnect v1

Context:
Pedro:

This would depend on the wallet… walletconnect allows any JSON-RPC methods

https://canary.discord.com/channels/492410046307631105/833628540456140821/905050546904399892
Reproduce:
https://github.com/AlphaWallet/alpha-wallet-ios/issues/3887#issuecomment-1034527245

Add in support for the switch and add Ethereum chain methods. See the 'Reproduce' above.
First, try it via the normal Dapp Browser with breakpoints at these two methods:
onWalletAddEthereumChainObject
onWalletSwitchEthereumChain
notice how when you are in the wrong chain these methods will be called, opening the relevant ActionSheet (showChainChangeDialog).
Implement these in WalletConnectActivity; you will need to debug the handleRequest method in WCClient.kt, the request should show up in there once it fires.
Pop open the dialog to get confirmation from the user. Note that hitting the 'x' or sliding the dialog down is rejection - no need for a reject button.
I think you can use updateSession similar to how it's done in WalletConnectService to change chains.
You will also need the infrastructure as per the code under the comment // show add custom chain dialog in DappBrowserFragment. The same dialog is fine to re-use (don't worry about implementing it via ActionSheet for now).
Have a look, let me know if you have any questions.

Hi @JamesSmartCell , I tried replicating the behavior but the switch chain action sheet is sometimes shown & sometimes not shown.  Also I don't see the wallet connect request in handleRequest method or do we need to add a session update call in showChainChangeDialog? sorry but I'm confused as I'm not able to fully understand.

To see the switch chain action you have to set the wallet onto the "wrong" chain, eg Ethereum Classic.
I will check to see if I can see the WalletConnect call for switch chain.

To see the switch chain action you have to set the wallet onto the "wrong" chain, eg Ethereum Classic.

I did try that, and it doesn't work,  It still shows Polygon network and on clicking Connect wallet, bottom dialog is opened but Wallet connect button is not provided.  But if I change the chain to Ethereum, switchChainChangeDialog is shown.

Also if I change chain from Uniswap interface, the switch dialog is shown twice and we are back to original chain.

Yeah I'm not sure it works the way we thought it did. If the app needs eg Polygon, it asks for Polygon when it connects.

Hi @hboon maybe you can help us here. I'm not sure when this would ever be relevant.
If the app needs Polygon (eg Quickswap, Pagaxies) then it asks for that at the time of WalletConnect init.
What circumstances are the 'change_chain' ever called for and is there any reproducible?

For me, it works perfectly any way I try it. EG if the dapp wants Gnosis, when I click on the WC connect it will ask to open in Gnosis.
etc etc.

Hi @hboon maybe you can help us here. I'm not sure when this would ever be relevant.
If the app needs Polygon (eg Quickswap, Pagaxies) then it asks for that at the time of WalletConnect init.
What circumstances are the 'change_chain' ever called for and is there any reproducible?

Not sure if it's still reproducible in Uniswap but see https://github.com/AlphaWallet/alpha-wallet-ios/issues/3887#issuecomment-1034527245
The use case:

User connect to dapp on chain X
User clicks in the dapp UI to change it to chain Y

I can think of another one, while bridging:

User connect to dapp on chain X
Bridges to chain Y
Dapp prompts used to switch to chain Y to poll for and show result

Another one, still for bridging. In li.fi's docs https://docs.li.finance/official-documentation/integrate-li.fi-natively/li.fi-sdk/advanced-features/handling-chain-switches

Some routes require a network switch in the end-users wallet during execution. This need arises when the route first bridges a token to another chain and subsequently wants to swap it into the final output token using an exchange.

So something like:

User connects to dapp on chain X
User select route: token T1 on chain X -> token T2 on chain Y
Dapp bridges T1 on chain X to T1 on chain Y
Dapp prompts to switch to chain Y in order to proceed to swap from T1 on chain Y to T2 on chain Y

I couldn't reproduce in Uniswap; it works correctly when trying to connect with a different chain.
I didn't really understand when @hieronymus4826 was saying that the Android connection didn't really work. Android serves whichever chain the dapp asks for when starting a session, and since there's no reproducible where a dapp wants to change chains I can't see where the issue is.

@hieronymus4826 can you still reproduce this for Android? I can't do it for both Android and iOS now: https://github.com/AlphaWallet/alpha-wallet-ios/issues/3887#issuecomment-1034527245

So I found that the "wallet_switchEthereumChain" is called by DAPP/uniswap when connected via wallet connect on desktop(or any other browser except AW) by scanning QR. when we select other chain on desktop, the mobile app will receive the wallet connect method "wallet_switchEthereumChain". I show a dialog, if user approves, we send "wc_sessionUpdate" with new chain Id approved, return null as the RPC response and the dapp will change chain. If user rejects, we send error response in RPC and dapp will show normal error message.

same for addEthereumChain, if user approves, we add the chain and update the session. The rpc request is returned null. If user rejects, then we respond with server error message: "rejected by user".


Install the config.h upon make install

Install a CppUTest prefixed config.h upon installation so that the system configuration can be kept there.

Coverage remained the same at 99.909% when pulling ca0dcaa3e0b3e165cc1bebaced616df4614c170c on basvodde:autotools_configure into 98e073635f2cc7d03967a7d42f5ef2e1fcbda145 on cpputest:master.

The PR isn't clean; it contains commits from other PRs.  And the one change that's in ca0dcaa just seems to be adding a file that is not actually used, so seems insufficient for the task.

The file is a m4 macro that is added to the configure.ac. Without it, it will fail :)

I'm not disputing that.  But configure.ac would need to be changed to use the file, as would any usage of the now-prefixed config.h macros.  Those changes are not present.

No, autoconf automatically loads the macros from the m4 directory. So the added line AX_ is actually using the macro

The line I can't find. Lemme check :)

Ok, now they are there.

There is no added AX_ line in the PR.  That is my point.  configure.ac has not been modified at all.

Yes, you were right :) I had not committed them. My mistake.

Coverage remained the same at 99.909% when pulling 21c515eb3b328a586c72b03e5401d440ebac7c74 on basvodde:autotools_configure into 98e073635f2cc7d03967a7d42f5ef2e1fcbda145 on cpputest:master.


Add sections?

Hello Alf!
I think paper-now is a great idea, congratulations! I tried to add more sections to those previously included in the folder, but unfortunately the page doesn't build correctly. I also changed sections.html to include the new files but it didn't work. Any suggestions?
Here's my repo if you want to check out the mess I did: https://github.com/danilofreire/invalid-votes
Thanks! :)

Hi Danilo :)
Thanks for being one of the first to experiment with using Paper Now to write a paper, and you're right - it was too difficult to add new sections.
I've improved the handling of the abstract, keywords and article sections, and sent a pull request to bring your repository up to date.

Hi again :)
Thanks a lot for fixing those issues. I'm looking at the files you've changed and everything seems to be alright. However, I think the page still doesn't build. I'll take a close look at the files and I get back to you soon.

It looks like it's built ok at http://danilofreire.com/invalid-votes/ - are you building locally, or on GitHub?

I tried to build on GitHub and it didn't work (I swear! haha). But now it works fine, thanks a lot. Problem solved :)

BTW I can highly recommend installing GitHub's Jekyll set-up locally to test-build your paper locally (https://help.github.com/articles/using-jekyll-with-pages/#installing-jekyll). That can save you a lot of broken commits, and I speak from experience :-)

You're right, Thomas. I'll do it next time! Thanks for the tip! :)