S3KeySensor to use S3Hook url parser - #21500

Merged
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation
Feb 15, 2022
Merged

S3KeySensor to use S3Hook url parser#21500
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation

Conversation

@dstandish

@dstandishdstandish commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

S3KeySensor resolves the bucket_name and key in the poke method.

Pulling this "resolution" logic out into a _resolve_bucket_and_key method makes it a little clearer what's happening there.

Unfortunately we can't just do it in __init__ because we have to wait for templates to render first.

I took the opportunity here to remove redundant s3 uri parsing logic, which already exists in a static method on the hook.

And since we use that hook method now, I had to update the tests to not mock the entire hook class but only the check methods.

Finally, since while I was at it I strengthened the test slightly, replacing assert op.poke(None) with assert op.poke(None) is True etc, since mocks can evaluate to "truthy"

Rather than relying on the poke method to parse the attrs we should either do it in `__init__` or in a cached property.
Since it's a pretty insignifigant computation let's just do it in `__init__`.
And anyway, if the params are bad then best to get a warning before deploying your code.
@josh-fell

Copy link
Copy Markdown
Contributor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

@josh-fell

josh-fell commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

FWIW, datatype checks on templated fields within __init__() have their issues as well. See #20347.

@dstandish

Copy link
Copy Markdown
ContributorAuthor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

darnit, good catch... maybe i'll go cached prop route

@dstandish
dstandish marked this pull request as draft February 10, 2022 19:11
@dstandishdstandish changed the title S3KeySensor should parse bucket and key outside of pokeS3KeySensor to use S3Hook url parserFeb 10, 2022
@dstandish
dstandish marked this pull request as ready for review February 11, 2022 06:37

@josh-felljosh-fell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit + a musing but LGTM

' should be relative path from root'
' level, rather than a full s3:// url'
)
raise AirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raiseAirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')
raiseAirflowException('If bucket_name is provided, bucket_key must be a relative path, not a URI.')

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to keep it on one line :) lemme see if that works

self.bucket_key = parsed_url.path.lstrip('/')
self.bucket_name, self.bucket_key = S3Hook.parse_s3_url(self.bucket_key)
else:
parsed_url = urlparse(self.bucket_key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but I wonder if S3Hook.parse_s3_url() could be augmented with some sort of configurable check between validating a URI vs. a relative path? Centralize some of this logic there without have urlparse() is different places. Anyway, just a thought.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish@josh-fell@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

S3KeySensor to use S3Hook url parser - #21500

Merged
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation
Feb 15, 2022
Merged

S3KeySensor to use S3Hook url parser#21500
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation

Conversation

@dstandish

@dstandishdstandish commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

S3KeySensor resolves the bucket_name and key in the poke method.

Pulling this "resolution" logic out into a _resolve_bucket_and_key method makes it a little clearer what's happening there.

Unfortunately we can't just do it in __init__ because we have to wait for templates to render first.

I took the opportunity here to remove redundant s3 uri parsing logic, which already exists in a static method on the hook.

And since we use that hook method now, I had to update the tests to not mock the entire hook class but only the check methods.

Finally, since while I was at it I strengthened the test slightly, replacing assert op.poke(None) with assert op.poke(None) is True etc, since mocks can evaluate to "truthy"

Rather than relying on the poke method to parse the attrs we should either do it in `__init__` or in a cached property.
Since it's a pretty insignifigant computation let's just do it in `__init__`.
And anyway, if the params are bad then best to get a warning before deploying your code.
@josh-fell

Copy link
Copy Markdown
Contributor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

@josh-fell

josh-fell commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

FWIW, datatype checks on templated fields within __init__() have their issues as well. See #20347.

@dstandish

Copy link
Copy Markdown
ContributorAuthor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

darnit, good catch... maybe i'll go cached prop route

@dstandish
dstandish marked this pull request as draft February 10, 2022 19:11
@dstandishdstandish changed the title S3KeySensor should parse bucket and key outside of pokeS3KeySensor to use S3Hook url parserFeb 10, 2022
@dstandish
dstandish marked this pull request as ready for review February 11, 2022 06:37

@josh-felljosh-fell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit + a musing but LGTM

' should be relative path from root'
' level, rather than a full s3:// url'
)
raise AirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raiseAirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')
raiseAirflowException('If bucket_name is provided, bucket_key must be a relative path, not a URI.')

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to keep it on one line :) lemme see if that works

self.bucket_key = parsed_url.path.lstrip('/')
self.bucket_name, self.bucket_key = S3Hook.parse_s3_url(self.bucket_key)
else:
parsed_url = urlparse(self.bucket_key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but I wonder if S3Hook.parse_s3_url() could be augmented with some sort of configurable check between validating a URI vs. a relative path? Centralize some of this logic there without have urlparse() is different places. Anyway, just a thought.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish@josh-fell@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

S3KeySensor to use S3Hook url parser - #21500

Merged
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation
Feb 15, 2022
Merged

S3KeySensor to use S3Hook url parser#21500
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation

Conversation

@dstandish

@dstandishdstandish commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

S3KeySensor resolves the bucket_name and key in the poke method.

Pulling this "resolution" logic out into a _resolve_bucket_and_key method makes it a little clearer what's happening there.

Unfortunately we can't just do it in __init__ because we have to wait for templates to render first.

I took the opportunity here to remove redundant s3 uri parsing logic, which already exists in a static method on the hook.

And since we use that hook method now, I had to update the tests to not mock the entire hook class but only the check methods.

Finally, since while I was at it I strengthened the test slightly, replacing assert op.poke(None) with assert op.poke(None) is True etc, since mocks can evaluate to "truthy"

Rather than relying on the poke method to parse the attrs we should either do it in `__init__` or in a cached property.
Since it's a pretty insignifigant computation let's just do it in `__init__`.
And anyway, if the params are bad then best to get a warning before deploying your code.
@josh-fell

Copy link
Copy Markdown
Contributor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

@josh-fell

josh-fell commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

FWIW, datatype checks on templated fields within __init__() have their issues as well. See #20347.

@dstandish

Copy link
Copy Markdown
ContributorAuthor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

darnit, good catch... maybe i'll go cached prop route

@dstandish
dstandish marked this pull request as draft February 10, 2022 19:11
@dstandishdstandish changed the title S3KeySensor should parse bucket and key outside of pokeS3KeySensor to use S3Hook url parserFeb 10, 2022
@dstandish
dstandish marked this pull request as ready for review February 11, 2022 06:37

@josh-felljosh-fell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit + a musing but LGTM

' should be relative path from root'
' level, rather than a full s3:// url'
)
raise AirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raiseAirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')
raiseAirflowException('If bucket_name is provided, bucket_key must be a relative path, not a URI.')

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to keep it on one line :) lemme see if that works

self.bucket_key = parsed_url.path.lstrip('/')
self.bucket_name, self.bucket_key = S3Hook.parse_s3_url(self.bucket_key)
else:
parsed_url = urlparse(self.bucket_key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but I wonder if S3Hook.parse_s3_url() could be augmented with some sort of configurable check between validating a URI vs. a relative path? Centralize some of this logic there without have urlparse() is different places. Anyway, just a thought.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish@josh-fell@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

S3KeySensor to use S3Hook url parser - #21500

Merged
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation
Feb 15, 2022
Merged

S3KeySensor to use S3Hook url parser#21500
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation

Conversation

@dstandish

@dstandishdstandish commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

S3KeySensor resolves the bucket_name and key in the poke method.

Pulling this "resolution" logic out into a _resolve_bucket_and_key method makes it a little clearer what's happening there.

Unfortunately we can't just do it in __init__ because we have to wait for templates to render first.

I took the opportunity here to remove redundant s3 uri parsing logic, which already exists in a static method on the hook.

And since we use that hook method now, I had to update the tests to not mock the entire hook class but only the check methods.

Finally, since while I was at it I strengthened the test slightly, replacing assert op.poke(None) with assert op.poke(None) is True etc, since mocks can evaluate to "truthy"

Rather than relying on the poke method to parse the attrs we should either do it in `__init__` or in a cached property.
Since it's a pretty insignifigant computation let's just do it in `__init__`.
And anyway, if the params are bad then best to get a warning before deploying your code.
@josh-fell

Copy link
Copy Markdown
Contributor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

@josh-fell

josh-fell commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

FWIW, datatype checks on templated fields within __init__() have their issues as well. See #20347.

@dstandish

Copy link
Copy Markdown
ContributorAuthor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

darnit, good catch... maybe i'll go cached prop route

@dstandish
dstandish marked this pull request as draft February 10, 2022 19:11
@dstandishdstandish changed the title S3KeySensor should parse bucket and key outside of pokeS3KeySensor to use S3Hook url parserFeb 10, 2022
@dstandish
dstandish marked this pull request as ready for review February 11, 2022 06:37

@josh-felljosh-fell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit + a musing but LGTM

' should be relative path from root'
' level, rather than a full s3:// url'
)
raise AirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raiseAirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')
raiseAirflowException('If bucket_name is provided, bucket_key must be a relative path, not a URI.')

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to keep it on one line :) lemme see if that works

self.bucket_key = parsed_url.path.lstrip('/')
self.bucket_name, self.bucket_key = S3Hook.parse_s3_url(self.bucket_key)
else:
parsed_url = urlparse(self.bucket_key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but I wonder if S3Hook.parse_s3_url() could be augmented with some sort of configurable check between validating a URI vs. a relative path? Centralize some of this logic there without have urlparse() is different places. Anyway, just a thought.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish@josh-fell@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

S3KeySensor to use S3Hook url parser - #21500

Merged
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation
Feb 15, 2022
Merged

S3KeySensor to use S3Hook url parser#21500
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation

Conversation

@dstandish

@dstandishdstandish commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

S3KeySensor resolves the bucket_name and key in the poke method.

Pulling this "resolution" logic out into a _resolve_bucket_and_key method makes it a little clearer what's happening there.

Unfortunately we can't just do it in __init__ because we have to wait for templates to render first.

I took the opportunity here to remove redundant s3 uri parsing logic, which already exists in a static method on the hook.

And since we use that hook method now, I had to update the tests to not mock the entire hook class but only the check methods.

Finally, since while I was at it I strengthened the test slightly, replacing assert op.poke(None) with assert op.poke(None) is True etc, since mocks can evaluate to "truthy"

Rather than relying on the poke method to parse the attrs we should either do it in `__init__` or in a cached property.
Since it's a pretty insignifigant computation let's just do it in `__init__`.
And anyway, if the params are bad then best to get a warning before deploying your code.
@josh-fell

Copy link
Copy Markdown
Contributor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

@josh-fell

josh-fell commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

FWIW, datatype checks on templated fields within __init__() have their issues as well. See #20347.

@dstandish

Copy link
Copy Markdown
ContributorAuthor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

darnit, good catch... maybe i'll go cached prop route

@dstandish
dstandish marked this pull request as draft February 10, 2022 19:11
@dstandishdstandish changed the title S3KeySensor should parse bucket and key outside of pokeS3KeySensor to use S3Hook url parserFeb 10, 2022
@dstandish
dstandish marked this pull request as ready for review February 11, 2022 06:37

@josh-felljosh-fell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit + a musing but LGTM

' should be relative path from root'
' level, rather than a full s3:// url'
)
raise AirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raiseAirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')
raiseAirflowException('If bucket_name is provided, bucket_key must be a relative path, not a URI.')

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to keep it on one line :) lemme see if that works

self.bucket_key = parsed_url.path.lstrip('/')
self.bucket_name, self.bucket_key = S3Hook.parse_s3_url(self.bucket_key)
else:
parsed_url = urlparse(self.bucket_key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but I wonder if S3Hook.parse_s3_url() could be augmented with some sort of configurable check between validating a URI vs. a relative path? Centralize some of this logic there without have urlparse() is different places. Anyway, just a thought.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish@josh-fell@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

S3KeySensor to use S3Hook url parser - #21500

Merged
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation
Feb 15, 2022
Merged

S3KeySensor to use S3Hook url parser#21500
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation

Conversation

@dstandish

@dstandishdstandish commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

S3KeySensor resolves the bucket_name and key in the poke method.

Pulling this "resolution" logic out into a _resolve_bucket_and_key method makes it a little clearer what's happening there.

Unfortunately we can't just do it in __init__ because we have to wait for templates to render first.

I took the opportunity here to remove redundant s3 uri parsing logic, which already exists in a static method on the hook.

And since we use that hook method now, I had to update the tests to not mock the entire hook class but only the check methods.

Finally, since while I was at it I strengthened the test slightly, replacing assert op.poke(None) with assert op.poke(None) is True etc, since mocks can evaluate to "truthy"

Rather than relying on the poke method to parse the attrs we should either do it in `__init__` or in a cached property.
Since it's a pretty insignifigant computation let's just do it in `__init__`.
And anyway, if the params are bad then best to get a warning before deploying your code.
@josh-fell

Copy link
Copy Markdown
Contributor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

@josh-fell

josh-fell commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

FWIW, datatype checks on templated fields within __init__() have their issues as well. See #20347.

@dstandish

Copy link
Copy Markdown
ContributorAuthor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

darnit, good catch... maybe i'll go cached prop route

@dstandish
dstandish marked this pull request as draft February 10, 2022 19:11
@dstandishdstandish changed the title S3KeySensor should parse bucket and key outside of pokeS3KeySensor to use S3Hook url parserFeb 10, 2022
@dstandish
dstandish marked this pull request as ready for review February 11, 2022 06:37

@josh-felljosh-fell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit + a musing but LGTM

' should be relative path from root'
' level, rather than a full s3:// url'
)
raise AirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raiseAirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')
raiseAirflowException('If bucket_name is provided, bucket_key must be a relative path, not a URI.')

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to keep it on one line :) lemme see if that works

self.bucket_key = parsed_url.path.lstrip('/')
self.bucket_name, self.bucket_key = S3Hook.parse_s3_url(self.bucket_key)
else:
parsed_url = urlparse(self.bucket_key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but I wonder if S3Hook.parse_s3_url() could be augmented with some sort of configurable check between validating a URI vs. a relative path? Centralize some of this logic there without have urlparse() is different places. Anyway, just a thought.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish@josh-fell@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

S3KeySensor to use S3Hook url parser - #21500

Merged
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation
Feb 15, 2022
Merged

S3KeySensor to use S3Hook url parser#21500
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation

Conversation

@dstandish

@dstandishdstandish commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

S3KeySensor resolves the bucket_name and key in the poke method.

Pulling this "resolution" logic out into a _resolve_bucket_and_key method makes it a little clearer what's happening there.

Unfortunately we can't just do it in __init__ because we have to wait for templates to render first.

I took the opportunity here to remove redundant s3 uri parsing logic, which already exists in a static method on the hook.

And since we use that hook method now, I had to update the tests to not mock the entire hook class but only the check methods.

Finally, since while I was at it I strengthened the test slightly, replacing assert op.poke(None) with assert op.poke(None) is True etc, since mocks can evaluate to "truthy"

Rather than relying on the poke method to parse the attrs we should either do it in `__init__` or in a cached property.
Since it's a pretty insignifigant computation let's just do it in `__init__`.
And anyway, if the params are bad then best to get a warning before deploying your code.
@josh-fell

Copy link
Copy Markdown
Contributor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

@josh-fell

josh-fell commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

FWIW, datatype checks on templated fields within __init__() have their issues as well. See #20347.

@dstandish

Copy link
Copy Markdown
ContributorAuthor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

darnit, good catch... maybe i'll go cached prop route

@dstandish
dstandish marked this pull request as draft February 10, 2022 19:11
@dstandishdstandish changed the title S3KeySensor should parse bucket and key outside of pokeS3KeySensor to use S3Hook url parserFeb 10, 2022
@dstandish
dstandish marked this pull request as ready for review February 11, 2022 06:37

@josh-felljosh-fell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit + a musing but LGTM

' should be relative path from root'
' level, rather than a full s3:// url'
)
raise AirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raiseAirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')
raiseAirflowException('If bucket_name is provided, bucket_key must be a relative path, not a URI.')

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to keep it on one line :) lemme see if that works

self.bucket_key = parsed_url.path.lstrip('/')
self.bucket_name, self.bucket_key = S3Hook.parse_s3_url(self.bucket_key)
else:
parsed_url = urlparse(self.bucket_key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but I wonder if S3Hook.parse_s3_url() could be augmented with some sort of configurable check between validating a URI vs. a relative path? Centralize some of this logic there without have urlparse() is different places. Anyway, just a thought.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish@josh-fell@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

S3KeySensor to use S3Hook url parser - #21500

Merged
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation
Feb 15, 2022
Merged

S3KeySensor to use S3Hook url parser#21500
potiuk merged 5 commits into
apache:mainfrom
astronomer:remove-s3-poke-attr-mutation

Conversation

@dstandish

@dstandishdstandish commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

S3KeySensor resolves the bucket_name and key in the poke method.

Pulling this "resolution" logic out into a _resolve_bucket_and_key method makes it a little clearer what's happening there.

Unfortunately we can't just do it in __init__ because we have to wait for templates to render first.

I took the opportunity here to remove redundant s3 uri parsing logic, which already exists in a static method on the hook.

And since we use that hook method now, I had to update the tests to not mock the entire hook class but only the check methods.

Finally, since while I was at it I strengthened the test slightly, replacing assert op.poke(None) with assert op.poke(None) is True etc, since mocks can evaluate to "truthy"

Rather than relying on the poke method to parse the attrs we should either do it in `__init__` or in a cached property.
Since it's a pretty insignifigant computation let's just do it in `__init__`.
And anyway, if the params are bad then best to get a warning before deploying your code.
@josh-fell

Copy link
Copy Markdown
Contributor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

@josh-fell

josh-fell commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

FWIW, datatype checks on templated fields within __init__() have their issues as well. See #20347.

@dstandish

Copy link
Copy Markdown
ContributorAuthor

Parsing bucket_key in the __init__() won't work if the value is an XComArg or a Jinja string. We've seen several issues, namely AttributeErrors, when some sort of string parsing is used on templated fields in __init__() since the value doesn't get rendered until the execute() method. Check out #14682 as an example.

darnit, good catch... maybe i'll go cached prop route

@dstandish
dstandish marked this pull request as draft February 10, 2022 19:11
@dstandishdstandish changed the title S3KeySensor should parse bucket and key outside of pokeS3KeySensor to use S3Hook url parserFeb 10, 2022
@dstandish
dstandish marked this pull request as ready for review February 11, 2022 06:37

@josh-felljosh-fell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit + a musing but LGTM

' should be relative path from root'
' level, rather than a full s3:// url'
)
raise AirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raiseAirflowException('If bucket_name provided, bucket_key must be relative path, not URI.')
raiseAirflowException('If bucket_name is provided, bucket_key must be a relative path, not a URI.')

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was trying to keep it on one line :) lemme see if that works

self.bucket_key = parsed_url.path.lstrip('/')
self.bucket_name, self.bucket_key = S3Hook.parse_s3_url(self.bucket_key)
else:
parsed_url = urlparse(self.bucket_key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but I wonder if S3Hook.parse_s3_url() could be augmented with some sort of configurable check between validating a URI vs. a relative path? Centralize some of this logic there without have urlparse() is different places. Anyway, just a thought.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish@josh-fell@potiuk