OCPBUGS-17545: Improve extracting opm binary from catalogs #676

sherine-k · 2023-08-16T13:15:25Z

Description

This PR improves the way the opm binary is extracted from catalogs by leveraging the ENTRYPOINT from the container image config.
It also forces pulling the container image that matches the runtime architecture and OS.

Fixes # OCPBUGS-17545

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

This was tested both on amd64 and arm64 OSs with:

 ./bin/oc-mirror -c iscv2.yaml docker://localhost:5000/aws --dest-use-http --dest-skip-tls

and the following config:

kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v1alpha2
mirror:
  # additionalImages: 
  #   - name: registry.redhat.io/ubi8/ubi:latest  
  operators:
    - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.13
      packages:
      - name: node-observability-operator
      # - name: aws-load-balancer-operator  
      # - name: 3scale-operator

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

openshift-ci-robot · 2023-08-16T13:15:31Z

@sherine-k: This pull request references Jira Issue OCPBUGS-17545, which is invalid:

expected the bug to target the "4.14.0" version, but it targets "4.14" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

New feature (non-breaking change which adds functionality)

Breaking change (fix or feature that would cause existing functionality to not work as expected)

This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A

Test B

Test Configuration:

Firmware version:

Hardware:

Toolchain:

SDK:

Checklist:

My code follows the style guidelines of this project

I have performed a self-review of my own code

I have commented my code, particularly in hard-to-understand areas

I have made corresponding changes to the documentation

My changes generate no new warnings

I have added tests that prove my fix is effective or that my feature works

New and existing unit tests pass locally with my changes

Any dependent changes have been merged and published in downstream modules

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2023-08-16T13:17:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sherine-k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sherine-k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-08-16T14:54:56Z

@sherine-k: This pull request references Jira Issue OCPBUGS-17545, which is invalid:

expected the bug to target the "4.14.0" version, but it targets "4.14" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Description

This PR improves the way the opm binary is extracted from catalogs by leveraging the ENTRYPOINT from the container image config.
It also forces pulling the container image that matches the runtime architecture and OS.

Fixes # OCPBUGS-17545

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

New feature (non-breaking change which adds functionality)

Breaking change (fix or feature that would cause existing functionality to not work as expected)

This change requires a documentation update

How Has This Been Tested?

This was tested both on amd64 and arm64 OSs with:
./bin/oc-mirror -c iscv2.yaml docker://localhost:5000/aws --dest-use-http --dest-skip-tls
and the following config:
kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v1alpha2
mirror:
 # additionalImages: 
 #   - name: registry.redhat.io/ubi8/ubi:latest  
 operators:
   - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.13
     packages:
     - name: node-observability-operator
     # - name: aws-load-balancer-operator  
     # - name: 3scale-operator
Checklist:

My code follows the style guidelines of this project

I have performed a self-review of my own code

I have commented my code, particularly in hard-to-understand areas

I have made corresponding changes to the documentation

My changes generate no new warnings

I have added tests that prove my fix is effective or that my feature works

New and existing unit tests pass locally with my changes

Any dependent changes have been merged and published in downstream modules

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sherine-k · 2023-08-16T14:55:59Z

/jira refresh

openshift-ci-robot · 2023-08-16T14:56:04Z

@sherine-k: This pull request references Jira Issue OCPBUGS-17545, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @zhouying7780

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lmzuccarelli · 2023-08-17T08:45:15Z

pkg/cli/mirror/catalog_images.go

+		return err
+	}
+	entrypoint := cfgf.Config.Entrypoint
+	opmBin := strings.TrimPrefix(entrypoint[0], (string)(os.PathSeparator))


are we confident of the fact that there will always be an entrypoint[0] - should we check first and error if we don't have the correct length ?

It's worth reading these docs to get a sense of what's possible for cmd and entrypoint:
https://docs.docker.com/engine/reference/builder/#cmd
https://docs.docker.com/engine/reference/builder/#entrypoint

This doc is also quite helpful in understanding how cmd and entry point can be used independently or together:
https://docs.docker.com/engine/reference/builder/#understand-how-cmd-and-entrypoint-interact

It's critical to understand the shell form vs. exec form, which applies to cmd and entrypoint. The table that shows how cmd and entry point interact is useful to see how things will behave.

lmzuccarelli · 2023-08-17T08:49:20Z

pkg/cli/mirror/catalog_images.go

-			return fmt.Errorf("opm binary not found after extracting %q from catalog image %v", opmBinaryExtracted, srcRef)
-		}
+	opmBinaryFileName := filepath.Join(ctlgSrcDir, config.OpmBinDir, "opm")
+	err = copyOPMBinary(img, opmBinaryFileName)


what about the windows-amd64-opm and darwin-amd64-opm binaries (is it ok to ignore them ?)

Hi @lmzuccarelli
opmBinaryFileName is the target file where we will store the opm binary corresponding to the runtime platform.
copyOPMBinary and more exactly extractOPMBinary will take care of extracting the right binary by looking at the entrypoint of the image in img parameter.
That source binary might be windows-amd64-opm or darwin-amd64-opm, but we're copying it as opm so that we simplify the following steps during processCatalogRefs as to call command opm under the catalogsource folder.
What do you think?

This was going to be my comment as well, this seems like we lost useful functionality.

I'd also like to see a mode where we can tell oc-mirror to use the already installed opm binary (e.g. --use-preinstalled-opm). I realize this is not something you might want to advertise, so maybe making a hidden flag for this option would be one way to go. My rationale for this is that when I am developing/testing I use a Mac, and any failure in finding a working opm executable is going to prevent me from using oc-mirror.

@sherine-k - thanks for the explanation !!

It just occurred to me that entrypoint is not going to work... here's why:

$ skopeo --override-os linux inspect docker://registry.redhat.io/openshift4/ose-operator-registry:v4.13 --config | jq '[.config.Entrypoint, .config.Cmd]' [ [ "/bin/registry-server" ], [ "--database", "/bundles.db" ] ]

The entrypoint has nothing to do with the opm executable.

$ skopeo --override-os linux inspect docker://registry.redhat.io/openshift4/ose-operator-registry:v4.13 --config | jq '[.config.Entrypoint, .config.Cmd]'

This is doesn't look like an FBC catalog. Is it?
If that is true, the fact that we only do all this logic when we detect --cache-dir in the CMD will allow us to skip all this for such catalogs.

This was going to be my comment as well, this seems like we lost useful functionality.

Can you explain why you think that?
In the previous implementation we were trying to blindly find anything *opm. In this implementation we are trying to extract the exact binary with which this image is running.

Regarding ose-operator-registry... I forgot that this is used as the base image in creating a new image that would have opm as the entrypoint. So disregard this comment. Doing too many things at once and did not think this through.

zhouying7780 · 2023-08-17T09:50:07Z

/label qe-approved

jchunkins · 2023-08-17T12:28:39Z

pkg/cli/mirror/catalog_images.go

-			return fmt.Errorf("opm binary not found after extracting %q from catalog image %v", opmBinaryExtracted, srcRef)
-		}
+	opmBinaryFileName := filepath.Join(ctlgSrcDir, config.OpmBinDir, "opm")
+	err = copyOPMBinary(img, opmBinaryFileName)


This was going to be my comment as well, this seems like we lost useful functionality.

I'd also like to see a mode where we can tell oc-mirror to use the already installed opm binary (e.g. --use-preinstalled-opm). I realize this is not something you might want to advertise, so maybe making a hidden flag for this option would be one way to go. My rationale for this is that when I am developing/testing I use a Mac, and any failure in finding a working opm executable is going to prevent me from using oc-mirror.

jchunkins · 2023-08-17T13:04:10Z

pkg/cli/mirror/catalog_images.go

+		return err
+	}
+	entrypoint := cfgf.Config.Entrypoint
+	opmBin := strings.TrimPrefix(entrypoint[0], (string)(os.PathSeparator))


It's worth reading these docs to get a sense of what's possible for cmd and entrypoint:
https://docs.docker.com/engine/reference/builder/#cmd
https://docs.docker.com/engine/reference/builder/#entrypoint

This doc is also quite helpful in understanding how cmd and entry point can be used independently or together:
https://docs.docker.com/engine/reference/builder/#understand-how-cmd-and-entrypoint-interact

It's critical to understand the shell form vs. exec form, which applies to cmd and entrypoint. The table that shows how cmd and entry point interact is useful to see how things will behave.

jchunkins · 2023-08-17T13:31:37Z

pkg/cli/mirror/catalog_images.go

+			// if err := os.Symlink(absLinkTarget, filepath.Join("/tmp", header.Name)); err != nil {
+			// 	panic(err)
+			// }
+			return extractOPMBinary(linkTarget, img, target)


You can't (or at least should not) run this recursively. This is going to force each depth of recursion to:

collapse the layers of the image into a flattened tar via mutate.Extract(img)

walk the whole file system tree (a catalog image can have thousands of files in it, most relating to the operating system)

Imagine the scenario where they did something like this:

/opm -> /foo/opm /foo/opm -> /bar/opm /bar/opm

Depending on the order these files are processed, you could get lucky and find /bar/opm first, or at worst have to do two levels of recursion before you find it.

Seems like this should be iterating once and keeping track of what you find along the way and then choose the "winner".

lmzuccarelli

@sherine-k - this I feel is a cleaner solution. I will leave @jchunkins to verify and also ensure his concerns were addressed

This new implementation extracts the whole catalog to a local folder in order to make it easier to grab the opm binary.

aguidirh · 2023-08-31T13:10:38Z

/lgtm

jchunkins · 2023-08-31T14:05:09Z

pkg/cli/mirror/catalog_images.go

+			// if it's a file create it
+			f, err := os.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode))
+			if err != nil {
+				return err
+			}
+
+			// copy over contents
+			if _, err := io.Copy(f, tr); err != nil {
+				return err
+			}
+
+			// manually close here after each file operation; defering would cause each file close
+			// to wait until all operations have completed.
+			f.Close()


Isn't this extracting every single file it encounters? Seems like you could check the header.Mode and see if its a least an executable before making the copy

jchunkins · 2023-08-31T14:58:27Z

pkg/cli/mirror/catalog_images.go

+
+	}
+	opmBinPathComponents := strings.Split(opmBin, string(filepath.Separator))
+	for link, src := range symLinks {


I am confused by the names here... maybe more documentation would help with understanding what you're doing.

As far as I can tell, the symLink map contains the following after iterating over the image contents:

symLink key == <destination folder>/<"name"> (i.e. this is the thing we might have just extracted ... see my other comment about header.Mode)
symLink value == <destination folder>/<"symlink name"> (i.e. this is just a path to something that does not exist yet)

Can you tell me what the values of header.Name or header.Linkname are? Is it just the base name (e.g. opm) or the path plus the file name (e.g. foo/opm)? I assume it's a path to a file or symlink, right?

Now when you iterate over linkMap, the variables don't seem to match, which is confusing me a bit:

link == <destination folder>/<path to file name> (i.e. an actual extracted file)
src == <destination folder>/<path to symlink name>

Let's discuss this in scrum since I am not following

What about the scenario where the path that is provided is a direct path to the binary? This seems like it assumes its going to be a symbolic link.

openshift-ci · 2023-09-05T10:23:30Z

@sherine-k: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

lmzuccarelli

overall this looks good. Great work @sherine-k

lmzuccarelli · 2023-09-05T11:40:41Z

pkg/cli/mirror/catalog_images.go

+	// Following https://docs.docker.com/engine/reference/builder/#understand-how-cmd-and-entrypoint-interact,
+	// identifying the binary that will be run by the catalog:
+	entrypoint := cfgf.Config.Entrypoint
+	if len(entrypoint) > 0 {


Nice - good example of defensive programming :)

lmzuccarelli · 2023-09-05T11:41:42Z

pkg/cli/mirror/catalog_images.go

+// All layers of the image are flattened.
+// All contents (files, folders and symLinks are extracted)
+// Errors related to creation of symbolic links are not considered as blockers
+// unless they relate to the `opmBin` path.


Comments are clear and concise

lmzuccarelli · 2023-09-05T11:42:36Z

pkg/cli/mirror/catalog_images.go

+			// descriptor=".../oc-mirror-workspace/src/catalogs/.../extracted/usr/bin/darwin-amd64-opm"; 		".../oc-mirror-workspace/src/catalogs/.../extracted/bin/registry/darwin-amd64-opm"
+			// descriptor=".../oc-mirror-workspace/src/catalogs/.../extracted/usr/bin/opm"; 					".../oc-mirror-workspace/src/catalogs/.../extracted/bin/registry/opm"
+			// descriptor=".../oc-mirror-workspace/src/catalogs/.../extracted/etc/alternatives/easy_install-3"; ".../oc-mirror-workspace/src/catalogs/.../extracted/usr/bin/easy_install-3.6"
+			// descriptor=".../oc-mirror-workspace/src/catalogs/.../extracted/usr/bin/easy_install-3.6"; 		".../oc-mirror-workspace/src/catalogs/.../extracted/hostname"


This is also good to have some examples of what physically can be expected

lmzuccarelli · 2023-09-05T11:43:43Z

@sherine-k - lets work towards getting this merged

lmzuccarelli · 2023-09-05T11:44:02Z

/lgtm

zhouying7780 · 2023-09-06T03:06:58Z

/label qe-approved

jerpeter1 · 2023-09-07T16:55:45Z

/label acknowledge-critical-fixes-only

openshift-ci-robot · 2023-09-07T16:58:46Z

@sherine-k: Jira Issue OCPBUGS-17545: All pull requests linked via external trackers have merged:

openshift/oc-mirror#676

Jira Issue OCPBUGS-17545 has been moved to the MODIFIED state.

In response to this:

Description

This PR improves the way the opm binary is extracted from catalogs by leveraging the ENTRYPOINT from the container image config.
It also forces pulling the container image that matches the runtime architecture and OS.

Fixes # OCPBUGS-17545

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

New feature (non-breaking change which adds functionality)

Breaking change (fix or feature that would cause existing functionality to not work as expected)

This change requires a documentation update

How Has This Been Tested?

This was tested both on amd64 and arm64 OSs with:
./bin/oc-mirror -c iscv2.yaml docker://localhost:5000/aws --dest-use-http --dest-skip-tls
and the following config:
kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v1alpha2
mirror:
 # additionalImages: 
 #   - name: registry.redhat.io/ubi8/ubi:latest  
 operators:
   - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.13
     packages:
     - name: node-observability-operator
     # - name: aws-load-balancer-operator  
     # - name: 3scale-operator
Checklist:

My code follows the style guidelines of this project

I have performed a self-review of my own code

I have commented my code, particularly in hard-to-understand areas

I have made corresponding changes to the documentation

My changes generate no new warnings

I have added tests that prove my fix is effective or that my feature works

New and existing unit tests pass locally with my changes

Any dependent changes have been merged and published in downstream modules

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…#676) * Improve extracting opm binary from catalogs * Fix OCPBUGS-17545 - Force pull catalog by platform architecture * Rewrite extractOPMBinary in a non-recursive way * Attempt 2 - Rewrite extractOPMBinary in a non-recursive way This new implementation extracts the whole catalog to a local folder in order to make it easier to grab the opm binary. * Account for cases where catalog image's entrypoint might be empty * Add comments to opm binary extraction logic

…o deploy because of invalid caches (#691) * CFE-825: As a oc-mirror user, I would like mirrored operator catalogs to have valid caches (#651) * Regenerate opm cache when building catalog images * Recreate the cache of local OCI catalogs * Incomplete - Include opm cache integrity verification to E2E Fix code regarding finding the location of the opm binary * TEMPORARY: use a different repository for source catalogs Fix E2E test catalog_full * Make cache regeneration during catalog rebuild conditional based on presence of argument `--cache-dir` in the original image container CMD * Fix review comments * OCPBUGS-17545: Improve extracting opm binary from catalogs (#676) * Improve extracting opm binary from catalogs * Fix OCPBUGS-17545 - Force pull catalog by platform architecture * Rewrite extractOPMBinary in a non-recursive way * Attempt 2 - Rewrite extractOPMBinary in a non-recursive way This new implementation extracts the whole catalog to a local folder in order to make it easier to grab the opm binary. * Account for cases where catalog image's entrypoint might be empty * Add comments to opm binary extraction logic * Fix E2E tests after conflict resolution

openshift-ci-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Aug 16, 2023

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Aug 16, 2023

openshift-ci bot requested review from aguidirh and jpower432 August 16, 2023 13:17

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2023

sherine-k changed the title ~~OCPBUGS-17545: WIP: Improve extracting opm binary from catalogs~~ OCPBUGS-17545: Improve extracting opm binary from catalogs Aug 16, 2023

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Aug 16, 2023

openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Aug 16, 2023

openshift-ci bot requested a review from zhouying7780 August 16, 2023 14:56

lmzuccarelli reviewed Aug 17, 2023

View reviewed changes

jchunkins suggested changes Aug 17, 2023

View reviewed changes

lmzuccarelli reviewed Aug 24, 2023

View reviewed changes

sherine-k added 5 commits August 28, 2023 10:56

Improve extracting opm binary from catalogs

0012c2c

Fix OCPBUGS-17545 - Force pull catalog by platform architecture

2fb3689

Rewrite extractOPMBinary in a non-recursive way

f12ca6b

Attempt 2 - Rewrite extractOPMBinary in a non-recursive way

18a7b85

This new implementation extracts the whole catalog to a local folder in order to make it easier to grab the opm binary.

Account for cases where catalog image's entrypoint might be empty

ccb512d

sherine-k force-pushed the OCPBUGS-17545 branch from 63cc5d1 to ccb512d Compare August 28, 2023 09:05

openshift-ci bot assigned aguidirh Aug 31, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2023

jchunkins suggested changes Aug 31, 2023

View reviewed changes

Add comments to opm binary extraction logic

dc5448d

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2023

lmzuccarelli reviewed Sep 5, 2023

View reviewed changes

openshift-ci bot assigned lmzuccarelli Sep 5, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2023

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 6, 2023

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Sep 7, 2023

openshift-merge-robot merged commit 00c48bb into openshift:main Sep 7, 2023
5 checks passed

sherine-k mentioned this pull request Sep 8, 2023

[release-4.13] OCPBUGS-18556: operator catalogs from oc-mirror fail to deploy because of invalid caches #691

Merged

OCPBUGS-17545: Improve extracting opm binary from catalogs #676

OCPBUGS-17545: Improve extracting opm binary from catalogs #676

Conversation

sherine-k commented Aug 16, 2023 • edited

Description

Type of change

How Has This Been Tested?

Checklist:

openshift-ci-robot commented Aug 16, 2023

Description

Type of change

How Has This Been Tested?

Checklist:

openshift-ci bot commented Aug 16, 2023

openshift-ci-robot commented Aug 16, 2023

Description

Type of change

How Has This Been Tested?

Checklist:

sherine-k commented Aug 16, 2023

openshift-ci-robot commented Aug 16, 2023

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

sherine-k Aug 17, 2023 • edited

Choose a reason for hiding this comment

sherine-k Aug 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouying7780 commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmzuccarelli left a comment

Choose a reason for hiding this comment

aguidirh commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 5, 2023

lmzuccarelli 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

lmzuccarelli commented Sep 5, 2023

lmzuccarelli commented Sep 5, 2023

zhouying7780 commented Sep 6, 2023

jerpeter1 commented Sep 7, 2023

openshift-ci-robot commented Sep 7, 2023

Description

Type of change

How Has This Been Tested?

Checklist:

sherine-k commented Aug 16, 2023 •

edited

sherine-k Aug 17, 2023 •

edited

sherine-k Aug 17, 2023 •

edited